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.

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

ui: Upgrade to Bootstrap 5 #1872

Merged
merged 6 commits into from
Oct 5, 2021
Merged

ui: Upgrade to Bootstrap 5 #1872

merged 6 commits into from
Oct 5, 2021

Conversation

thi4go
Copy link
Member

@thi4go thi4go commented Sep 30, 2021

This diff upgrades bootstrap to v5.1.1 and applies the necessary changes on our UI code to keep things the same.

UI changes

Edit: no more UI changes

CHANGELOG

Bootstrap 5 started using start and end instead of left and right. Implied on the following changes:

  • text-left to text-start
  • text-right to text-end
  • ml-n (margin-left) to ms-n (margin-start)
  • mr-n (margin-right) to me-n (margin-end)
  • pl-n (padding-left) to ps-n (padding-start)
  • pr-n (padding-right) to pe-n (padding-end)

Changes on some form-control class variables, which have been overriden to keep consistency with the current UI design:

  • $input-color: $gray-700 !default; to $input-color: $body-color !default;
  • $input-bg: $white !default; to $input-bg: $body-bg !default;

Changes on the nav-link class where a color property was added. This was overriden as well.

Path import for some bootstrap modules dcrdata uses were changed. Please see public/scss/application.scss for detailed changes on this.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

The reason the boldness is changed is that font-weight-bold no longer exists in v5 (it's in the Utilities section of the migration guide). We need to change all font-weight-bold to fw-bold in all the .tmpl and .js.
We also need to edit typography.scss to use .fw-bold .decimal-parts .int { near the end of the file.

If we do want to tweak the bold weight anyway, we can do so in public/scss/_variables.scss in the // typography section:

$font-weight-bold:      600; // instead of 700

Note that the variable used inside bootstrap is still $font-weight-bold, but the utility is now fw-bold

Finally, let's do a git rm public/scss/_variables_reference.scss. It was originally just a copy of Bootstrap's _variables.scss (bootstrap/scss/_variables.scss), but it is so out of date as to be useless. Plus it's just as easy to access the actual one in node_modules.

cmd/dcrdata/public/scss/_variables.scss Outdated Show resolved Hide resolved
cmd/dcrdata/public/scss/application.scss Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented Sep 30, 2021

It's a bit of a shame that the bundle (css/style.css) is now about 20 kB larger. Are we 100% sure each of those bootstrap imports is required? The finer granularity of the imports should actually help us reduce the bundle size.

@chappjc
Copy link
Member

chappjc commented Oct 1, 2021

It's a bit of a shame that the bundle (css/style.css) is now about 20 kB larger. Are we 100% sure each of those bootstrap imports is required? The finer granularity of the imports should actually help us reduce the bundle size.

@thi4go No need to investigate this comment... I spent some time on it and I have a couple ideas, but there's potential for breakage so I want to test every page carefully. Let's just wrap up with an iteration on my review and we can merge.

@thi4go
Copy link
Member Author

thi4go commented Oct 1, 2021

@chappjc thx for the review mate, pushed the changes that addresses the comments 👍

@chappjc
Copy link
Member

chappjc commented Oct 1, 2021

Notice a couple deviations on /decodetx

Before

image

image

image

After

image

image

image

The form itself is no longer 100% width. It appears that col has change. However, we were only using col for the width here (not flex as the (i) tooltip will tell us), so let's make it something more appropriate.

The second thing is the buttons. The text color seems to have changed.

@chappjc
Copy link
Member

chappjc commented Oct 1, 2021

Also can you please prefix ~ to the bootstrap links. Although docs say that's deprecated, I cannot nav to them anymore from my editor, and without the ~, that seems to instruct webpack to try relative to current path first and then the node_modules folder, but we know which one it is.

The loader will first try to resolve @import as a relative path. If it cannot be resolved, then the loader will try to resolve @import inside node_modules.

Prepending module paths with a ~ tells webpack to search through node_modules.

@import "~bootstrap";

It's important to prepend it with only ~, because ~/ resolves to the home directory. Webpack needs to distinguish between bootstrap and ~bootstrap because CSS and Sass files have no special syntax for importing relative files. Writing @import "style.scss" is the same as @import "./style.scss";

So while the sass-loader docs say you can remove it and it is deprecated, it seems like it still has a powerful use case, and indeed it breaks my editing experience.

@thi4go
Copy link
Member Author

thi4go commented Oct 1, 2021

Oh right, will return the ~ prefix on the imports. I was afraid something would pass me unnoticed since there was so many details to be aware of. Thx for catching those mate. Will be back to keyboard in a few and will push the fix.

@chappjc
Copy link
Member

chappjc commented Oct 1, 2021

Another one on charts:

Before

image

After

image

The above seems to be related to a change in form-control, but it's not clear.

I'm wondering if the primary color has changed somehow?

@chappjc
Copy link
Member

chappjc commented Oct 1, 2021

The <br> on this legend has no width now, weird:

image

@chappjc chappjc merged commit 90e57d2 into decred:master Oct 5, 2021
$input-bg: #fff;
$input-color: #495057;
$nav-link-color: inherit;
// $min-contrast-ratio: 3.9; // contrasting color for the primary (Decred blue) depends on contrast factor
Copy link
Member

@chappjc chappjc Oct 5, 2021

Choose a reason for hiding this comment

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

For posterity, we talked a lot about the Decode button's text color and it came down to the change in Bootstrap's color contast system.
https://getbootstrap.com/docs/5.0/migration/#sass

Renamed color-yiq() function and related variables to color-contrast() as it’s no longer related to YIQ colorspace

What that doesn't explain is that the new WCAG algo potentially changes the contrasting color that gets picked, in this case for Decred blue.

https://github.com/twbs/bootstrap/blob/57d80fcd3274ba1ff0a1580966e098fda80f74e7/scss/_variables.scss#L97
https://getbootstrap.com/docs/5.0/customize/sass/#color-contrast
https://www.w3.org/TR/WCAG20/#visual-audio-contrast-contrast

In the end, I concluded it was pretty much an accident that this button was using white as the text color, and it really should have the same text as the other (Broadcast) button and with an high contrast in terms of WCAG.

So now the text color for the Decode button is black, and we have this code comment.

@chappjc chappjc added this to the 6.1 milestone Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants