-
Notifications
You must be signed in to change notification settings - Fork 118
Issue #3144 - Upgrade to Bootstrap 5 #3374
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
Conversation
This commit consists of the file changes required for the upgrade from Bootstrap 3 to 5. A more detailed version is in CHANGELOG.md (and in wiki: https://github.com/DMPRoadmap/roadmap/wiki/Release-notes-for-Bootstrap-5-upgrade)
Generated by 🚫 Danger |
|
Rubocop fixes to follow soon. |
|
We found in the Template copy link. We will fix. The modal first appears without link. Only on second attempt modal appears with link. It appears that the link is not available during the initialisation phase roadmap/app/javascript/src/publicTemplates/show.js Lines 1 to 15 in 932e31e
|
|
I tried merging the Fix MySQL and PostgreSQL GitHub Actions PR into this PR. There are a couple of noteworthy things after running the rspec tests: Failure/Error: expect(page).to have_text('0/1 answered')(Probably related to a change in Failure/Error: expect(rendered.include?('modal-search-result-selector hidden')).to eql(true)
|
|
Hi @aaronskiba thank you for spotting this. I am working on a fix for the second one and @johnpinto1 will look into the first one and we should sort it out soon. |
Changes in this PR: 1) modal-search-result-selector class changed from 'hidden' to 'd-none' 2) changes to the related rspec tests for the above
app/assets/stylesheets/application.scss - Changed `@use "variables";` to `@use "variables" as *;` This enables our overrides to take effect when the Bootstrap stylesheets are imported. - Changed `@use` to `@import` for blocks and utils and moved the statements to be executed after the Bootstrap imports. This enables the customisations within these files to take effect. All app/assets/stylesheets/blocks/_x.scss files that included `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` - Changed `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` to `@import '../../../../node_modules/bootstrap-sass/assets/stylesheets/bootstrap/variables';` The former statement was undoing our overrides specified within `app/assets/stylesheets/variables/` app/assets/stylesheets/blocks/_navbars.scss - Added `!optional` to `@extend .navbar-inverse;`. The removal of `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` necessitated this change (the same approach is being used in the following DMPRoadmap PR: DMPRoadmap#3374) All public/tinymce/skins/oxide/ files - These were auto-generated after executing `rake assets:clobber && rake assets:precompile`
app/assets/stylesheets/application.scss - Changed `@use "variables";` to `@use "variables" as *;` This enables our overrides to take effect when the Bootstrap stylesheets are imported. - Changed `@use` to `@import` for blocks and utils and moved the statements to be executed after the Bootstrap imports. This enables the customisations within these files to take effect. All app/assets/stylesheets/blocks/_x.scss files that included `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` - Changed `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` to `@import '../../../../node_modules/bootstrap-sass/assets/stylesheets/bootstrap/variables';` The former statement was undoing our overrides specified within `app/assets/stylesheets/variables/` app/assets/stylesheets/blocks/_navbars.scss - Added `!optional` to `@extend .navbar-inverse;`. The removal of `@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as *;` necessitated this change (the same approach is being used in the following DMPRoadmap PR: DMPRoadmap#3374) All public/tinymce/skins/oxide/ files - These were auto-generated after executing `rake assets:clobber && rake assets:precompile`
1: Added to spec/rails_helper.rb:
Capybara.default_max_wait_time = 10
2: Fix for SuperAdmins Orgs Super admin removes links
Failure/Error: all('.link a > .fa-circle-xmark').last.click
Changed selectors in spec/features/super_admins/orgs_spec.rb & updated spec/support/helpers/links_helper.rb updated. It was necessary, to scroll to links that were not visible and click by using Javascripts scripts.
3: Fixed errors in ./spec/features/questions/textarea_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
4: Fix for error in ./spec/features/annotations/annotations_editing_spec.rb
Changed "Noo bar" -> "<p>Noo bar</p>".
5: Fixed error in ./spec/features/questions/radiobuttons_questions_spec.rb.
Changed "(0/1) answered" -> "(0 / 1)".
6: Fixed error in ./spec/features/questions/dropdown_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)".
7: Fixed error in spec/features/feedback_requests_spec.rb
Replaced "within('div.panel') do" -> "within('.tab-pane.active')"
8: Fixed errors in ./spec/features/questions/checkbox_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
9: Fixed error in ./spec/features/templates/templates_editings_spec.rb.
Changed "Foo bar" -. "<p>Foo bar</p>".
10: Fix for ./spec/features/questions/textfield_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
|
@aaronskiba I have partially fixed some of the Rspec tests. I have 6 tests still to fix. 6 of these consistently fail although in exports-spec.rb the tests that fail vary with each run. locales_spec.rb tests fail sometimes.
|
This commit consists of the file changes required for the upgrade from Bootstrap 3 to 5. A more detailed version is in CHANGELOG.md (and in wiki: https://github.com/DMPRoadmap/roadmap/wiki/Release-notes-for-Bootstrap-5-upgrade)
Changes in this PR: 1) modal-search-result-selector class changed from 'hidden' to 'd-none' 2) changes to the related rspec tests for the above
1: Added to spec/rails_helper.rb:
Capybara.default_max_wait_time = 10
2: Fix for SuperAdmins Orgs Super admin removes links
Failure/Error: all('.link a > .fa-circle-xmark').last.click
Changed selectors in spec/features/super_admins/orgs_spec.rb & updated spec/support/helpers/links_helper.rb updated. It was necessary, to scroll to links that were not visible and click by using Javascripts scripts.
3: Fixed errors in ./spec/features/questions/textarea_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
4: Fix for error in ./spec/features/annotations/annotations_editing_spec.rb
Changed "Noo bar" -> "<p>Noo bar</p>".
5: Fixed error in ./spec/features/questions/radiobuttons_questions_spec.rb.
Changed "(0/1) answered" -> "(0 / 1)".
6: Fixed error in ./spec/features/questions/dropdown_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)".
7: Fixed error in spec/features/feedback_requests_spec.rb
Replaced "within('div.panel') do" -> "within('.tab-pane.active')"
8: Fixed errors in ./spec/features/questions/checkbox_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
9: Fixed error in ./spec/features/templates/templates_editings_spec.rb.
Changed "Foo bar" -. "<p>Foo bar</p>".
10: Fix for ./spec/features/questions/textfield_questions_spec.rb.
Changed "(0/1) answered" -> "( 0 / 1)",
"(1/1) answered" -> "( 1 / 1)"
…to1:DMPRoadmap/roadmap into issue_3144_upgrade_to_bootstrap_5
- rubocop fixes after Bootstrap upgrade and RSpec tests fixes (Note: default value for 'dropdown' argument removed in app/helpers/customizable_template_link_helper.rb as part of rubocop fix)
|
Note: As part of the Rubocop fixes commit the default value for It is called in two places in the code and it is always called with a value for |
- Request feedback box changed from 'well' to card' class - Reset password view alignment of checkbox and label fields - Required field red star alignment for Project Details and Share plan pages
aaronskiba
left a comment
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.
I'm very sorry, I had made several review comments for this PR, but I guess I never clicked submit to make them visible. :(
DMP Assistant is finally getting around to merging this release. If there is anything worth addressing in my now published comments or elsewhere, I will definitely contribute them back to this repository.
| title: _("Click to select %{item_name}") % { item_name: title } %> | ||
| <%= link_to _("Remove"), '#', | ||
| class: "modal-search-result-unselector #{selected ? '' : 'hidden'}", | ||
| class: "modal-search-result-unselector #{selected ? '' : 'd-none'}", |
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.
I think that this particular update also needs to be applied within app/javascript/src/utils/modalSearch.js. (additional info: #3374 (comment))
| @import "../../../node_modules/bootstrap/scss/type"; | ||
| @import "../../../node_modules/bootstrap/scss/images"; | ||
| @import "../../../node_modules/bootstrap/scss/containers"; | ||
| @import "../../../node_modules/bootstrap/scss/grid"; |
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.
I think the overriding of bootstrap variables is working as it should here (this seems to solve some issues I'm encountering while merging release v4.1.0 into DMP Assistant).
However, there may be some redundant import statements here. https://getbootstrap.com/docs/5.0/customize/sass/#importing states "You have two options: include all of Bootstrap, or pick the parts you need." I think we may be unnecessarily be adding both options.
e.g. node_modules/bootstrap/scss/bootstrap.scss includes the following:
@import "functions";
@import "variables";
@import "maps";
@import "mixins";
@import "utilities";
// Layout & components
@import "root";
@import "reboot";
@import "type";
@import "images";
@import "containers";
@import "grid";| @@ -1,12 +1,8 @@ | |||
| // Import locally defined variables. Load this before 'bootstrap' | |||
|
|
|||
| @use "variables"; | |||
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.
I need to learn more about the SASS rules, but I worry about this being @use "variables";, rather than @use "variables" as *; (or maybe @import "variables";)
// app/assets/stylesheets/variables/_bootstrap.scss
$link-decoration: none !default;// node_modules/bootstrap/scss/_variables.scss
$link-decoration: underline !default;I think we want our $link-decoration: none !default; to override the Bootstrap default.
// .gitignore
# /app/assets/builds/* Commented out see diff while testing
I ran rake assets:clobber && rake assets:precompile with @use "variables"; and commited the changes.
Next, I ran rake assets:clobber && rake assets:precompile with @use "variables" as *;. Here are some of the resultant differences in app/assets/builds/application.css (it looks like some of those Bootstrap overrides were being missed):
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.
CHANGELOG.md mentions "Links are underlined by default (not just on hover), unless they're part of specific components. So we had to add css to remove underline in many cases."
If we implement the above change, maybe we can remove the extra css that was added to handle the underlines?
|
|
||
| // Panels | ||
| $panel-border-radius: 0px !default; | ||
| $panel-border-radius: 0rem !default; |
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.
I'm wondering if some things should be changed in this file.
I'm looking at the variables defined innode_modules/bootstrap/scss/_variables.scss, and comparing them to the variables defined in the same file for Bootstrap 3.4.1:
$input-border-radius exists in both Bootstrap versions. Are we sure we want to delete that variable here?
$navbar-border-radius no longer exists in Bootstrap 5. So it should be good to delete like you did.
$btn-border-radius-base appears to now be $btn-border-radius. Should we make the same update here?
$btn-primary-color no longer exists in Bootstrap 5. Maybe we should delete this one?
$btn-default-color no longer exists in Bootstrap 5. Neither does $btn-secondary-color. Rather than the rename, maybe this one can be deleted as well?
$panel-border-radius is now $card-border-radius in Bootstrap 5 (I think cards have replaced panels)
| @import "../../../node_modules/bootstrap/scss/grid"; | ||
|
|
||
| //Importing customised stylesheets after Bootstrap defaults | ||
| @import "blocks"; |
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.
Not really due to this PR, but I noticed that the following ends up being added to app/assets/builds/application.css twice:
@font-face {
font-family: 'GillSansLight';
src: url(asset-path('GillSans-Light.ttf')) format("truetype");
font-weight: normal;
}This code comes from app/assets/stylesheets/variables/_typography.scss. It must be getting added twice because of the way at-rules are handled. Also, app/assets/stylesheets/variables/_index.scss includes @forward 'typography' and @use '../variables/typography' as *; is mentioned in some of the app/assets/stylesheets/blocks/ files. Maybe the at-rule and the SASS variable in app/assets/stylesheets/variables/_typography.scss should each exist in separate directories?
| @use "utils"; | ||
|
|
||
| // Pull in the webpacker managed copy of Bootstrap Stylesheets | ||
| @import "../../../node_modules/bootstrap-sass/assets/stylesheets/bootstrap"; |
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.
We may be able to simplify all of the node_modules imports within .scss files.
We're using the cssbundling-rails gem, and the README includes the following:
How do I include 3rd party stylesheets from node_modules in my bundle?
Use an @import statement and path to a specific stylesheet, omitting the node_modules/ segment and the file's extension. For example:
/* Desired file is at at node_modules/select2/dist/css/select2.css */
@import "select2/dist/css/select2";| } | ||
|
|
||
| /* RADIO BUTTONS */ | ||
| .radio label { |
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.
The CHANGELOG.md states "Renamed checkbox and radio into form-check." Did we also have to perform that renaming here (app/assets/stylesheets/blocks/_labels.scss) and in app/assets/stylesheets/blocks/_html.scss?










Following is a brief list of changes made by @johnpinto1 and @gjacob24 for the upgrade to Bootstrap 5.
A more detailed version is in CHANGELOG.md (and in wiki: https://github.com/DMPRoadmap/roadmap/wiki/Release-notes-for-Bootstrap-5-upgrade)
Summary of changes:
Node package changes:
- Changed version of bootstrap "^3.4.1" --> "^5.2.3"
- Added @popperjs/core.
- Removed bootstrap-3-typeahead, bootstrap-sass & popper.js
Stylesheet changes:
- In
app/assets/stylesheets/application.scss:removed bootstrap-sass import and replaced with
@import "../../../node_modules/bootstrap/scss/bootstrap".- The order of the import statements have been changed to import the
blocks/andutils/after the default bootstrap stylesheets- In
app/assets/stylesheets/blocks/:replaced in relevant files
@use "../../../../node_modules/bootstrap-sass/assets/stylesheets/_bootstrap.scss" as * ;with
@use "../../../../node_modules/bootstrap/scss/bootstrap" as *;Bootstrap usage changes (look at CHANGELOG.mdorn wiki for details.). Many of these changes requires Javascripot changes too.
- Bespoke versions replaced by Bootstrap 5 accordion and spinner.
- Data attributes now use
-bs-as required by Bootstrap 5 (used by accordion and dropdown buttons):data-display-->data-bs-displaydata-parent-->data-bs-parentdata-target-->data-bs-targetdata-toggle-->data-bs-toggle- Bootstrap 5 Popover added to some dropdown-menu items by adding attribute
data-bs-toggle="popover".- Panels, thumbnails & wells have been dropped
- Navs & navbars have been rewritten to follow changes in Bootsrap 5.
- Notifications now use classes
d-blockandd-noneto show and hide respectively.- The hidden and show classes have been removed because they conflicted with Jquery's. Replaced hidden with
d-none.