Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DEP Upgrade bootstrap and reactstrap #1889

Open
wants to merge 1 commit into
base: 3.0
Choose a base branch
from
Open

DEP Upgrade bootstrap and reactstrap #1889

wants to merge 1 commit into from

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli marked this pull request as draft January 15, 2025 03:26
@GuySartorelli GuySartorelli mentioned this pull request Jan 15, 2025
Open
3 tasks
@GuySartorelli GuySartorelli force-pushed the pulls/3.0/upgrade-bootstrap branch 9 times, most recently from ac8f8e9 to 3f0d527 Compare January 22, 2025 01:45
@GuySartorelli GuySartorelli force-pushed the pulls/3.0/upgrade-bootstrap branch from 3f0d527 to 2634745 Compare January 22, 2025 22:22
@GuySartorelli GuySartorelli marked this pull request as ready for review January 23, 2025 00:02
@GuySartorelli GuySartorelli force-pushed the pulls/3.0/upgrade-bootstrap branch from 2634745 to fcdaea0 Compare January 23, 2025 00:20
GuySartorelli
GuySartorelli commented Jan 23, 2025
View reviewed changes
Copy link
Member Author

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to point out the reasoning behind each type of change exactly once - do ask if reasoning isn't clear for any change.

client/src/styles/_bootstrap-variables.scss
//$link-decoration: none !default;
$link-decoration: none !default;
$link-hover-color: darken($link-color, 5%);
//$link-hover-decoration: underline !default;
$link-hover-decoration: underline !default;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reverts the following noted change from the bootstrap upgrade guide:

Links are underlined by default (not just on hover), unless they’re part of specific components

We may want to follow that same change in the future but for now the intention is to not alter the existing styling for the most part - and this represented a significant (and inconsistent due to our existing custom css) change in styling.

client/src/styles/_bootstrap-variables.scss
@@ -364,7 +367,7 @@ $input-btn-padding-y-sm: .3077rem; // 4px

// Forms

//$input-bg: $white !default;
$input-bg: $white !default;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverts a change that gave regular form inputs the same bg colours as the body, which was grey and therefore looked like disabled or read-only inputs

code/LeftAndMain.php
@@ -349,11 +349,10 @@ protected function init()

// Bootstrap components
Requirements::javascript('silverstripe/admin: thirdparty/popper/popper.min.js');
Requirements::javascript('silverstripe/admin: thirdparty/bootstrap/js/dist/util.js');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per https://github.com/orgs/twbs/discussions/33152 the util.js file is no longer needed

webpack.config.js
Comment on lines +52 to +67
// Bootstrap components - can't just rely on CopyWebpackPlugin for these because some of them require
// additional files that aren't declared in the documentation.
new JavascriptWebpackConfig('bootstrap', PATHS, 'silverstripe/admin')
.setEntry(glob.sync(`${PATHS.MODULES}/bootstrap/js/dist/**/*.js`).reduce((obj, el) => {
const parsedPath = path.parse(el);
const dir = parsedPath.dir.replace(new RegExp(`${PATHS.MODULES}\/bootstrap\/js\/dist\/?`), '');
obj[path.join(dir, parsedPath.name)] = el;
return obj;
}, {}))
.mergeConfig({
output: {
path: `${path.resolve('thirdparty')}/bootstrap/js/dist`,
filename: '[name].js',
},
})
.getConfig(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files weren't actually getting updated when bootstrap was updated! Now they will be.

client/src/components/Accordion/AccordionBlock.js
data-toggle="collapse"
data-bs-toggle="collapse"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://getbootstrap.com/docs/5.0/migration/#javascript

Data attributes for all JavaScript plugins are now namespaced to help distinguish Bootstrap functionality from third parties and your own code. For example, we use data-bs-toggle instead of data-toggle.

Other data-* attributes that need changing seem to only need to be changed if data-bs-toggle is also present - see this third-party script which seems to list all the relevant attributes (I didn't run this, just used it as a guide)

client/src/styles/legacy/_forms.scss
Comment on lines -33 to +34
label.right {
label.right,
label.end {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted to retain styling on original class here in case I missed something in markup, and because reactstrap has opted to continue supporting right and left for the time being.

client/src/styles/legacy/_forms.scss
.urlsegment .cancel {
margin-top: 3px;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This margin broke the styling of urlsegment fields

code/LeftAndMain.php
Comment on lines -356 to +355
"window.jQuery('body').tooltip({ selector: '[data-toggle=tooltip]' });",
"window.jQuery('[data-bs-toggle=tooltip]').tooltip();",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code started reacting badly, using jquery-ui tooltips instead of bootstrap ones even after updating the selector.
New code is recommended in https://getbootstrap.com/docs/5.2/getting-started/javascript/#optionally-using-jquery

templates/SilverStripe/Admin/Includes/LeftAndMain_EditForm.ss
Comment on lines -60 to +62
<% include SilverStripe\\Admin\\LeftAndMain_ViewModeSelector SelectID="preview-mode-dropdown-in-content", ExtraClass="ml-0" %>
<% include SilverStripe\\Admin\\LeftAndMain_ViewModeSelector SelectID="preview-mode-dropdown-in-content", ExtraClass="ms-0" %>
<% else %>
<% include SilverStripe\\Admin\\LeftAndMain_ViewModeSelector SelectID="preview-mode-dropdown-in-content", ExtraClass="ml-auto" %>
<% include SilverStripe\\Admin\\LeftAndMain_ViewModeSelector SelectID="preview-mode-dropdown-in-content", ExtraClass="ms-auto" %>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://getbootstrap.com/docs/5.0/migration/#utilities

Renamed .ml-* and .mr-* to .ms-* and .me-*.

webpack.config.js
Comment on lines -25 to +27
from: `${PATHS.MODULES}/popper.js/dist/umd/popper.js`,
from: `${PATHS.MODULES}/@popperjs/core/dist/umd/popper.min.js`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrap 5 uses popper2 not popper1

This was referenced Jan 23, 2025
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Jan 23, 2025
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@GuySartorelli