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

fix(design): Updated CSS selectors for dark scrollbar colour [JOB-108461] #2106

Merged
merged 4 commits into from
Nov 7, 2024
Merged

fix(design): Updated CSS selectors for dark scrollbar colour [JOB-108461] #2106

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a43cb04
scrollbar fix
Beccaneer Nov 5, 2024
a56ec73
updating selectors
Beccaneer Nov 5, 2024
e003596
readding previous setting
Beccaneer Nov 6, 2024
b848239
fixing selector
Beccaneer Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/design/src/styles/dark.mode.suffixStyles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
/* autoprefixer: off */
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this autoprefixer comment doing anything previously that we don't need it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I think I should add it back since we are keeping the current code for the safari use case 👍 Thanks 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding it back! This was to prevent problems when bundling inside JO IIRC... but I don't remember which specific part was being prefixed.

However, this /* autoprefixer: off */ is for a block of code, not the entire file as far as I know. It needs to be moved directly above @supports not (scrollbar-color: #000 #000) { on line 10 to be effective I think. And as a precaution, let's leave it here too, I think we want it applied to both blocks.

I'll find some time to test this and verify the output styles are the same 👍

@supports selector(::-webkit-scrollbar) {
@supports (scrollbar-color: #000 #000) {
html[data-theme="dark"] {
scrollbar-color: var(--color-surface--background--subtle)
var(--color-surface--background);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is looking great! I'm still testing other browsers, but I love it so far 😄

Screen.Recording.2024-11-06.at.11.18.27.AM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Firefox on production wasn't actually using the webkit selector? But on local, it's now working as expected!

Screen.Recording.2024-11-06.at.11.50.24.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you so much for QAing

Copy link
Contributor

Choose a reason for hiding this comment

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

Np, I usually don't upload all my QA steps like this, but posting here so Chris can see them in case he doesn't get the chance to run this.

I'm just going to test browserstack after my next meeting and then we should be good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we need the real proof it works, not like that fake proof I added before 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Windows edge (chromium) and firefox both look good too 👍

windows.edge.mov
windows.firefox.mov


/* Safari doesn't support scrollbar-color yet, so we're using the non-standard selector for now. */
/* autoprefixer: off */
@supports not (scrollbar-color: #000 #000) {
[data-theme="dark"]:root html ::-webkit-scrollbar,
[data-theme="dark"]:root body ::-webkit-scrollbar {
background-color: var(--color-surface--background);
Copy link
Contributor

@jdeichert jdeichert Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

✅ Safari is behaving as it currently does on production, using the webkit selectors

Screen.Recording.2024-11-06.at.11.23.36.AM.mov

Expand Down
Loading