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

Feature: Cross browser styled scrollbars #492

Merged

Conversation

napieralla
Copy link
Contributor

@napieralla napieralla commented Aug 1, 2020

Replace all native-scrollbars with CSS-styled ones.

New scrollbars for:

  • PageContent class (generic page wrapper)
  • Modal class (for album popups)
  • ArtistList class (list of artists saved to library)
  • Embedded lyrics-popup (Genius lyrics iframe)
  • Queue-popup (for songs in play-queue)

Remove the class-specific scrollbar CSS properties and adds a single scrollbarWrapper class in common.scss.

Other SASS classes extend this class for its scrollbar-related properties.
This removes duplicate code and makes it easier to add custom scrollbar styling to future components.

Add CSS properties for styling Firefox scrollbars.

Previous properties were all for webkit-powered browsers.
Firefox only supports very basic scrollbar properties and thus no corner-radius styling is possible.

Make scrollbars slightly darker when pressed and held.

This adds visual feedback when scrolling by dragging the scrollbar.

Increase the thickness of the narrowest webkit-scrollbars from 3 to 4px.

I personally found the horizontal scrollbars on the For You and Browse pages difficult to click with a mouse and I still think they look good at 4px, while being a little more accessible.
Such are the perils of not using trackspads and magic mice ;-)

Screenshots here.


P.S.

I have only had the chance to test these changes in Firefox and Chromium, so please let me know if something doesn't look right in Safari.

@netlify
Copy link

netlify bot commented Aug 1, 2020

Deploy request for musish pending review.

Review with commit ee36897

https://app.netlify.com/sites/musish/deploys

@napieralla
Copy link
Contributor Author

napieralla commented Aug 1, 2020

I chose not to include screenshots because there are so many minor changes that I expect you will want to see and test them for yourself.

Screenshots below.

@napieralla
Copy link
Contributor Author

There is by default no option to offset scrollbars from their containers, which means no padding between the scrollbar and the edge of the CSS-container.
This stands out and doesn't look good as it breaks spacing conventions used elsewhere in the app.
The above 4 commits address this by adding an extra wrapper-div, where necessary, around scrollable content and then padding that wrapper-div to offset scrollbars from their containers.
This way we get that "floating" scrollbar look that Apple scrollbars are known for.

@napieralla
Copy link
Contributor Author

Firefox before:

image

Firefox after:

image

Chromium before:

image

Chromium after:

image

@napieralla napieralla changed the title Feature: Better scrollbar support Feature: Cross browser styled scrollbars Aug 2, 2020
@napieralla
Copy link
Contributor Author

Bump - @BrychanOdlum
Would you have the time to inspect this PR any time soon?

@BrychanOdlum
Copy link
Contributor

BrychanOdlum commented Aug 15, 2020

Bump - @BrychanOdlum
Would you have the time to inspect this PR any time soon?

Hey, sorry for the slow response. Thanks for the great PR, this is definitely a step in the right direction.

I was testing this on Firefox v79.0 (macOS 11b3), and I noticed that the album & song grid scrollbars don't seem to be styled as demoed above. Other browsers seem fine. I'm assuming this is a macOS specific issue. Is this something you can look into?

[Firefix:79.0, macOS 11b3]:
Screenshot 2020-08-15 at 14 05 17

Separately, but the pageContent's scrollbars don't seem be styling for me either (macOS 11b3) across each of Safari, Chrome, and Firefox:

[Chrome:75, macOS 11b3]:
Screenshot 2020-08-15 at 14 07 08

This isn't a regression, and so if you're not working on macOS, then I'd be happy to approve this as-is for now. Otherwise, let me know if you're able to work on this or are getting different results.

@napieralla
Copy link
Contributor Author

Thanks for testing the 3 browsers on macOS.
I only tested this on Linux and I was under the impression that Firefox and Chrome would produce virtually the same output regardless of OS, and that Safari would look similar to Chrome.

I'm definitely not happy with the above look and I'm curious as to how both the color of and padding around the scrollbars are wrong. I will try to get my hands on a macOS computer so that I can fix these issues before merging.

@BrychanOdlum
Copy link
Contributor

BrychanOdlum commented Aug 15, 2020

Thanks for testing the 3 browsers on macOS.
I only tested this on Linux and I was under the impression that Firefox and Chrome would produce virtually the same output regardless of OS, and that Safari would look similar to Chrome.

I'm definitely not happy with the above look and I'm curious as to how both the color of and padding around the scrollbars are wrong. I will try to get my hands on a macOS computer so that I can fix these issues before merging.

Hey again, sorry! It turns out I'd ran yarn in the wrong directory after checking out your project!
DfKL_yPX4AAAaPG

@BrychanOdlum
Copy link
Contributor

BrychanOdlum commented Aug 15, 2020

Okay, Chrome and Safari on macOS are now looking great!

Only nitpick on these is that the page content's scrollbar is a tiny bit too wide compared to the default macOS style.

[Safari 14.0, macOS11b3]:
Screenshot 2020-08-15 at 15 25 26

Firefox, on the other hand, still doesn't seem to want to conform to the new rules though.

[Firefox 79, macOS 11b3]:
Screenshot 2020-08-15 at 15 30 10

Update: ideally on macOS it'd be great to have the scrollbar as the default 14px(?) wide, and when hovered over go to 22px(?). But, to be fair, the 18px(?) it seems to be now is a good middle ground.

@BrychanOdlum
Copy link
Contributor

Okay, the plot thickens. It turns out Firefox on macOS does support scrollbar-color, but only when your macOS system settings has the Show scroll bars option set to always (System Preferences > General). 🤷‍♂️

So, I think this PR is as good as we're going to get unless we want to spend time changing the width of scrollbars on hover, which isn't really necessary... probably?

@napieralla
Copy link
Contributor Author

Hey again, sorry! It turns out I'd ran yarn in the wrong directory after checking out your project!

No worries, that happens :)

Update: ideally on macOS it'd be great to have the scrollbar as the default 14px(?) wide, and when hovered over go to 22px(?).

There is currently an equal amount of padding on all sides of the scrollbars, which would not be the case if the scrollbar grew on hover. I therefore suggest we refrain from changing the thickness dynamically. That's also an Apple-specific thing so Windows and Linux users won't know to miss it.

But, to be fair, the 18px(?) it seems to be now is a good middle ground.

So we will leave it as it is? I might be in the minority here but I like the (kinda) thick full-page scrollbars.

Okay, the plot thickens. It turns out Firefox on macOS does support scrollbar-color, but only when your macOS system settings has the Show scroll bars option set to always (System Preferences > General).

Thanks for resolving that. Does this also change the positioning of the scrollbars?
It seems macOS or Safari overrides the actual scrollbar settings by removing the scrollbar element unless the container is hovered. Then upon hovering it replaced with a default-styled scrollbar that hovers over the content.

See these pictures, the first pic is with the aforementioned behaviour and the second pic is how I want it to look (notice the padding on both sides of the scrollbar).
image
image

@raphaelvigee raphaelvigee merged commit 2da79b5 into Musish:master Aug 15, 2020
@raphaelvigee
Copy link
Member

Thanks all for the contribution 😁🚀

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.

3 participants