-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump-Bootstrap 4.6.2 - 5.3.3 #409
base: main
Are you sure you want to change the base?
Bump-Bootstrap 4.6.2 - 5.3.3 #409
Conversation
WalkthroughThis pull request represents a comprehensive upgrade from Bootstrap 4 to Bootstrap 5, involving multiple components and configuration files. The changes primarily focus on updating data attributes, class names, and import/usage syntax across the project. Package versions for key dependencies like Bootstrap, Font Awesome, and Sass have been updated to their latest versions. The modifications aim to ensure compatibility with Bootstrap 5's new conventions while maintaining the existing functionality of the application. Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@tachyons Kindly have a look when you get time. |
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@tachyons May you please also reply to an idea referring to this on personal over slack ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Navbar/User/User.vue (1)
Line range hint
2-14
: Update deprecated Bootstrap classes.Several Bootstrap 3/4 classes need to be updated to their Bootstrap 5 equivalents:
- <ul class="nav navbar-nav noSelect pointerCursor pull-right account-btn"> + <ul class="nav navbar-nav noSelect pointerCursor float-end account-btn"> - <li v-if="authStore.getIsLoggedIn === true" class="dropdown pull-right"> + <li v-if="authStore.getIsLoggedIn === true" class="dropdown float-end"> <a href="#" class="cur-user acc-drop user-field" data-bs-toggle="dropdown" role="button" aria-haspopup="true" aria-expanded="false" - >{{ authStore.getUsername }}<span class="caret acc-caret"></span + >{{ authStore.getUsername }}<span class="dropdown-toggle acc-caret"></span ></a>
🧹 Nitpick comments (2)
src/components/Navbar/Hamburger/Hamburger.vue (1)
6-7
: Update the target ID to follow Bootstrap 5 naming conventions.While the data attributes have been correctly updated to Bootstrap 5's
data-bs-*
prefix, the target IDbs-example-navbar-collapse-1
follows the old Bootstrap 4 naming convention. Consider using a more semantic ID name.- data-bs-target="#bs-example-navbar-collapse-1" + data-bs-target="#navbarContent"src/components/Navbar/NavbarLinks/NavbarLink/NavbarLink.vue (1)
4-6
: Remove empty class attribute.The anchor tag contains an empty class attribute which can be removed.
- class="" data-bs-toggle="dropdown"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.json
(3 hunks)src/components/DialogBox/SaveImage.vue
(1 hunks)src/components/Dropdown/DropDown.vue
(1 hunks)src/components/Navbar/Hamburger/Hamburger.vue
(1 hunks)src/components/Navbar/Navbar.vue
(1 hunks)src/components/Navbar/NavbarLinks/NavbarLink/NavbarLink.vue
(1 hunks)src/components/Navbar/QuickButton/QuickButton.vue
(1 hunks)src/components/Navbar/QuickButton/QuickButtonMobile.vue
(1 hunks)src/components/Navbar/User/User.vue
(1 hunks)src/components/ReportIssue/ReportIssue.vue
(4 hunks)src/components/ReportIssue/ReportIssueButton.vue
(1 hunks)src/styles/simulator.scss
(1 hunks)vite.config.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/components/DialogBox/SaveImage.vue
- src/components/Navbar/Navbar.vue
- src/components/Navbar/QuickButton/QuickButtonMobile.vue
- src/components/Navbar/QuickButton/QuickButton.vue
🔇 Additional comments (9)
src/components/Navbar/User/User.vue (1)
Line range hint
1-1
: Additional Bootstrap 5 migration tasks needed.While the data attributes have been correctly updated to use the
data-bs-*
prefix, there are several other Bootstrap 5 migration tasks that should be addressed:
- Update class selectors for modals to use ID selectors
- Replace deprecated classes with their Bootstrap 5 equivalents
- Update naming conventions for component IDs
- Review and update any JavaScript initialization code (if any)
These changes will ensure full compatibility with Bootstrap 5 and maintain consistency across the codebase.
Let's verify if there are any other Bootstrap 4 classes or data attributes that need updating:
src/components/Dropdown/DropDown.vue (1)
33-33
: LGTM! Correctly updated to Bootstrap 5's data attribute prefix.The change from
data-method
todata-bs-method
follows Bootstrap 5's new naming convention for data attributes.vite.config.ts (1)
27-37
: Verify timeline for removing temporary SCSS configuration.The added SCSS configuration handles Bootstrap's color function deprecations. While this works, it's marked as temporary.
Please track when Bootstrap updates their import methods to remove this configuration. You can monitor Bootstrap's progress on this issue by:
✅ Verification successful
Keep SCSS configuration until Bootstrap v6 release
The temporary SCSS configuration for color function deprecations is still required with Bootstrap 5.x. Based on recent releases (up to v5.3.3), Bootstrap hasn't fully addressed these deprecations yet. They plan to make significant Sass-related changes in v6, but no specific timeline is available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest Bootstrap releases and their changelog for SCSS-related updates gh api graphql -f query=' { repository(owner: "twbs", name: "bootstrap") { releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { tagName description } } } }'Length of output: 14030
src/components/ReportIssue/ReportIssue.vue (3)
18-23
: Close button updated correctly for Bootstrap 5.The changes follow Bootstrap 5's new conventions:
- Changed class from
close
tobtn-close
- Updated to
data-bs-dismiss
- Removed unnecessary inner content (Bootstrap 5 handles the close icon via CSS)
44-44
: Form group classes updated correctly.Changed
form-group
tomb-3
following Bootstrap 5's utility-first approach.Also applies to: 69-69
84-84
: Modal dismiss attribute updated correctly.Updated to
data-bs-dismiss
following Bootstrap 5's new data attribute prefix convention.src/styles/simulator.scss (1)
1-4
: LGTM! Modernized SCSS imports.Updated to use modern SCSS module system:
- Changed from
@import
to@use
- Properly configured font path
- Added appropriate namespacing
package.json (2)
26-26
: Verify Bootstrap version addresses the security vulnerability.The upgrade to Bootstrap 5.3.3 is higher than the version mentioned in the PR description (5.3.2). Please verify that this version properly addresses CVE-2024-6531.
Run this script to check the Bootstrap version and its security advisories:
✅ Verification successful
Bootstrap 5.3.3 is secure and appropriate.
The version is legitimate and newer than initially proposed (5.3.2). No active security vulnerabilities are found for Bootstrap 5.3.3, and all previously known issues were patched in the 5.x series. The vulnerabilities shown in the audit are for development dependencies and unrelated to Bootstrap itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Bootstrap version and security advisories # Check npm for latest versions and security advisories npm view bootstrap versions --json | jq '.[-5:]' # Show last 5 versions npm audit [email protected] # Check GitHub security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "bootstrap") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2355
Line range hint
16-58
: Review the impact of Bootstrap major version upgrade.The upgrade from Bootstrap 4 to 5 is a major version change that includes breaking changes. This requires:
- Comprehensive testing of all UI components
- Updates to Bootstrap-dependent components (class names, data attributes)
- Verification of custom styles compatibility
Run this script to identify potential Bootstrap 4 to 5 migration issues:
data-bs-toggle="modal" | ||
data-bs-target=".issue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use ID selector for modal target.
Bootstrap 5 recommends using ID selectors instead of class selectors for modal targets. This provides better specificity and prevents potential conflicts.
- data-bs-target=".issue"
+ data-bs-target="#issueModal"
Also ensure the corresponding modal element has a matching ID:
<div class="modal" id="issueModal">
@@ -13,7 +13,7 @@ | |||
"test": "vitest" | |||
}, | |||
"dependencies": { | |||
"@fortawesome/fontawesome-free": "^6.5.1", | |||
"@fortawesome/fontawesome-free": "^6.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid Font Awesome version specified.
The specified version ^6.7.2
appears to be incorrect as the latest stable version is 6.5.1. This version number might be a typo or might not exist.
Please update to a valid version:
- "@fortawesome/fontawesome-free": "^6.7.2",
+ "@fortawesome/fontawesome-free": "^6.5.1",
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
"@fortawesome/fontawesome-free": "^6.7.2", | |
"@fortawesome/fontawesome-free": "^6.5.1", |
$fa-font-path: '../../node_modules/@fortawesome/fontawesome-free/webfonts'; | ||
@import '../../node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss'; | ||
@import '../../node_modules/@fortawesome/fontawesome-free/scss/solid.scss'; | ||
@use '@fortawesome/fontawesome-free/scss/fontawesome' with ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been done in response to the recent changes in the sass guidelines and the deprecation of the import methods
preprocessorOptions: { | ||
scss: { | ||
includePaths: ['node_modules'], | ||
sassOptions: { // For the color function deprecations. To remove after future bootstrap updates ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to silence some of the warning, which are due to bootstrap not being up to date with the sass guidelines, bootstrap will improve these in it's future updates
This PR brings forth
Bootstrap bump from 4.6.2 to 5.3.2 answering to CVE-2024-6531
Vulnerability mitigation in
Summary by CodeRabbit
Release Notes
Dependencies
UI/UX Changes
data-bs-*
syntaxStyling
@use
module systemThese updates modernize the project's frontend framework and improve overall styling consistency.