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

Dark texts #1038

Merged
merged 16 commits into from
Feb 14, 2022
Merged

Dark texts #1038

merged 16 commits into from
Feb 14, 2022

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Jan 13, 2022

I tried to manage all cases in that few lines, feel free to debate or try some things on this branch about dark texts. Need to see about .bg-white into a .bg-dark.

Preview : No preview at the moment, just add .bg-dark to <body>.

Solutions tested :

  • Need to add :not([class*="bg-"]) ? (to get [class*="-dark"] :not([class*="bg-"]) {}) Tested and not concluant atm, maybe to see with a :has in the future (when implmented) ?
  • I like the CSS variables solution as it doesn't introduce more rules and follow the Bootstrap ones. On the other side, it means that every change of background needs to introduce variables changes for text or need to be done with utilities.

Need to see if there is some rollback to do on carousel, dropdowns and so on ?

@louismaximepiton louismaximepiton linked an issue Jan 14, 2022 that may be closed by this pull request
2 tasks
@louismaximepiton louismaximepiton marked this pull request as ready for review January 20, 2022 16:23
@julien-deramond
Copy link
Member

julien-deramond commented Jan 27, 2022

Added some examples to test the limits with b0ea83c.
Live preview

@julien-deramond
Copy link
Member

Sounds rather good to me.

Some edge cases:

  • Nested white (in CSS) background divs inside a .bg-dark div aren't working directly. But the developer isn't totally blocked since text utilities like .text-body can be used. However that's not the case for the links and the codes.
  • With supporting background colors, I suppose that sometimes the code color isn't right and definitely the hovered links color isn't right neither. One solution with utilities could be to use .link-light.

scss/_reboot.scss Outdated Show resolved Hide resolved
@julien-deramond
Copy link
Member

@yannickcornaille or @Lausselloic, anything to add before we begin to clean up this PR?

@yannickcornaille
Copy link
Member

All this suits me.
I noticed a small display problem with the links contained in an element with class .text-muted (example on the home page). I don't know whether to deal with this particular case or to remove the background background-color: #fff.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Not 100% sure about this solution but this is the best option found so far.

@Lausselloic Does your spider-sense warn you about something or can I merge? (after having fixed pa11y issues of course)

Copy link
Member

@Lausselloic Lausselloic left a comment

Choose a reason for hiding this comment

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

solution sounds quite good for me. Maybe some adjustment need to be done.
Don't know if @ffoodd have an advice on `[class*="..."] selector? Is there any performance issues or warning?

scss/_reboot.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
@ffoodd
Copy link
Contributor

ffoodd commented Feb 7, 2022

Nope, however Bootstrap's stylelint config will warn you about it.

I personnally prefer attribute selector to overuse classes. 👌

scss/_reboot.scss Outdated Show resolved Hide resolved
scss/_reboot.scss Outdated Show resolved Hide resolved
scss/_reboot.scss Outdated Show resolved Hide resolved
scss/_reboot.scss Outdated Show resolved Hide resolved
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@julien-deramond julien-deramond merged commit 8302566 into main Feb 14, 2022
@julien-deramond julien-deramond deleted the main-lmp-dark-texts branch February 14, 2022 07:08
@julien-deramond julien-deramond mentioned this pull request Feb 14, 2022
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.

<code> on dark backgrounds
5 participants