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

Docs: group together reusable CSS for examples in a single stylesheet #35649

Merged

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Jan 4, 2022

Description

  • Gathering reusable CSS from Bootstrap examples in site/layouts/_default/examples.html
  • This stylesheet defines .b-example-divider, .b-example-vr and .bi used in several examples.

Motivation & Context

This change could help the maintenance of the examples by gathering in a single place some CSS definitions reused several times in Bootstrap examples.

Types of changes

  • Refactoring (non-breaking change)

Checklist

Related issues

Live previews

For non-regression testing:

@julien-deramond julien-deramond requested a review from a team as a code owner January 4, 2022 12:27
@julien-deramond julien-deramond changed the title Docs: group together examples reusable CSS in a stylesheet Docs: group together reusable CSS for examples in a single stylesheet Jan 4, 2022
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Sounds OK to me but it needs some more testing. I think CSS @import is still considered a bad practice regarding performances, isn't it?

BTW I think some examples' styles could be replaced with new utilities. Another topic 😄

.bi {
vertical-align: -.125em;
pointer-events: none;
Copy link
Member

Choose a reason for hiding this comment

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

This leftover could be dropped by using the pointer-events utility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed via 045faaf. I'll look into the main feedback about @import asap.

@ffoodd
Copy link
Member

ffoodd commented Jan 5, 2022

@XhmikosR @GeoSot any thought regarding @import for such a small file? Should we link it from the <head> maybe?

@GeoSot
Copy link
Member

GeoSot commented Jan 5, 2022

<style>
.bd-placeholder-img {
font-size: 1.125rem;
text-anchor: middle;
-webkit-user-select: none;
-moz-user-select: none;
user-select: none;
}
@media (min-width: 768px) {
.bd-placeholder-img-lg {
font-size: 3.5rem;
}
}
</style>

As it already has a shared style, maybe is fine to move it there

@julien-deramond
Copy link
Member Author

julien-deramond commented Jan 5, 2022

I've just tested 2 different approaches by disabling cache and enabling throttling (Regular 2G). The @import version appears to be longer to be downloaded: 14.71s. The reason is that the loading is sequential: example.css is loaded after dropdown.css.

When removing the @import and rather do it directly in the dropdowns/index.html (it will be inserted as a <link>):

extra_css:
  - "../examples.css"
  - "dropdowns.css"

both the CSS files are loaded at the same time and the global downloaded time of the CSS files is 8.34s.

It seems that the 2nd approach would be better for the 1st load of the examples.css.

With cache enabled it doesn't make any difference after the 1st load.

@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

Definitely not import. It's bad practice. We can instead inline it on build timel

@ffoodd
Copy link
Member

ffoodd commented Jan 5, 2022

@GeoSot 's suggestion is good but could lead to side effects in other examples if those classes are used without such styles.

Could worth a try 🙂

@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2022

@@ -31,6 +31,28 @@
font-size: 3.5rem;
}
}

.b-example-divider {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ffoodd I didn't see it at first but the sidebars example displays vertical separators. So I've created here a common class grouping together color, box shadow and border. And then 2 separator classes allowing to have an horizontal one and a vertical one.

There are several ways of doing it: only use 2 classes, only put the .b-example-divider horizontal in this file and let examples like sidebars overwrite it to make a vertical one, etc. I'd like your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I think .b-example-hr could be dropped. Its height can be the default, and only .b-example-vr would override it.

Apart from this suggestion, it looks fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 1b51e6d

Comment on lines 1 to 2
.opacity-50 { opacity: .5; }
.opacity-75 { opacity: .75; }
Copy link
Member

Choose a reason for hiding this comment

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

I guess those should be dropped since utilities exist already. Maybe another PR later to refactor such styles?

Copy link
Member Author

@julien-deramond julien-deramond Jan 18, 2022

Choose a reason for hiding this comment

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

Yes I will create another branch + PR to try to refactor it after the merge of this one 👌

@@ -31,6 +31,28 @@
font-size: 3.5rem;
}
}

.b-example-divider {
Copy link
Member

Choose a reason for hiding this comment

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

I think .b-example-hr could be dropped. Its height can be the default, and only .b-example-vr would override it.

Apart from this suggestion, it looks fine.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM 👌

Any other considerations, @GeoSot and @XhmikosR?

@GeoSot
Copy link
Member

GeoSot commented Jan 19, 2022

LGTM too

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

LGTM too, thanks!

@XhmikosR XhmikosR merged commit 520cc8d into twbs:main Jan 19, 2022
@XhmikosR XhmikosR removed the css label Jan 19, 2022
@julien-deramond julien-deramond deleted the main-jd-add-common-examples-stylesheet branch January 19, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants