Copy bootstrap scss code#258
Conversation
Summary of ChangesHello @PieterjanDeClippel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's styling capabilities by integrating the full Bootstrap SCSS framework into a new, dedicated Nx library. This strategic move allows for greater control over Bootstrap's appearance, facilitates custom theming, and streamlines the compilation process of these styles into optimized CSS outputs. The change lays the groundwork for consistent and responsive UI development across the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
b5d72f9 to
7db1e5c
Compare
|
View your CI Pipeline Execution ↗ for commit 7db1e5c
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Code Review
This pull request introduces a new library mintplayer-bootstrap by copying the SCSS source code from the Bootstrap project. It also includes the necessary configuration to build the CSS files from the SCSS sources. My review focuses on the integration aspects and dependency updates.
I've identified a couple of areas for improvement:
- The build commands in
project.jsonare highly repetitive, which could be a maintenance concern. I've suggested refactoring this into a script. - The
package.jsonfile updates Prettier to a new major version (v3), which has breaking changes. I've added a comment to highlight the potential impact and recommend a migration strategy.
Overall, the approach of vendoring Bootstrap SCSS is clear, but the implementation details could be improved for better long-term maintainability.
| @@ -82,7 +83,7 @@ | |||
| "postcss-import": "^14.1.0", | |||
| "postcss-preset-env": "^7.5.0", | |||
| "postcss-url": "^10.1.3", | |||
There was a problem hiding this comment.
You are upgrading Prettier from v2 to v3. This is a major version upgrade and includes breaking changes. It's important to be aware of these changes as they might affect code formatting across the entire repository.
I recommend checking the Prettier v3 migration guide to understand the impact.
After this upgrade, it would be a good practice to run Prettier on all files in the repository to ensure consistent formatting. This is often best done in a separate, dedicated PR to keep this change isolated.
| "commands": [ | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap.scss dist/libs/mintplayer-bootstrap/css/bootstrap.css --style=expanded --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap.scss dist/libs/mintplayer-bootstrap/css/bootstrap.min.css --style=compressed --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-grid.scss dist/libs/mintplayer-bootstrap/css/bootstrap-grid.css --style=expanded --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-grid.scss dist/libs/mintplayer-bootstrap/css/bootstrap-grid.min.css --style=compressed --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-reboot.scss dist/libs/mintplayer-bootstrap/css/bootstrap-reboot.css --style=expanded --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-reboot.scss dist/libs/mintplayer-bootstrap/css/bootstrap-reboot.min.css --style=compressed --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-utilities.scss dist/libs/mintplayer-bootstrap/css/bootstrap-utilities.css --style=expanded --source-map", | ||
| "sass libs/mintplayer-bootstrap/scss/bootstrap-utilities.scss dist/libs/mintplayer-bootstrap/css/bootstrap-utilities.min.css --style=compressed --source-map" | ||
| ], |
There was a problem hiding this comment.
The build commands for sass are very repetitive. This could make it difficult to maintain in the future. For example, if you need to add a new file or change a sass option, you'll have to update it in multiple places, which is error-prone.
I suggest moving these commands into a separate build script (e.g., a shell script or a Node.js script). This would centralize the build logic, making it easier to manage and update.
For example, you could have a script like scripts/build-bootstrap.js and then just call it from here: "command": "node scripts/build-bootstrap.js".
This would look something like this:
// scripts/build-bootstrap.js
const { execSync } = require('child_process');
const files = [
'bootstrap',
'bootstrap-grid',
'bootstrap-reboot',
'bootstrap-utilities',
];
const sourceRoot = 'libs/mintplayer-bootstrap/scss';
const outputRoot = 'dist/libs/mintplayer-bootstrap/css';
files.forEach(file => {
const inputFile = `${sourceRoot}/${file}.scss`;
const outputFile = `${outputRoot}/${file}.css`;
const minOutputFile = `${outputRoot}/${file}.min.css`;
execSync(`sass ${inputFile} ${outputFile} --style=expanded --source-map`, { stdio: 'inherit' });
execSync(`sass ${inputFile} ${minOutputFile} --style=compressed --source-map`, { stdio: 'inherit' });
});This is just an example, a shell script would also work well.
No description provided.