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

Make userlist section titles sticky on scroll, improve contrasts for accessibility, use CSS variables #2643

Merged
merged 5 commits into from
Jul 25, 2018

Conversation

astorije
Copy link
Member

@astorije astorije commented Jul 13, 2018

Stickiness extracted from #2527. Also slightly updated styling, and improved accessibility of muted texts on default theme (via the new CSS var) and was able to get rid of some overrides in Morning due to that CSS var.

Browsers that do not support position: sticky essentially do nothing with it (i.e. treat it as an element positioned by default) so that's good enough graceful degradation. It's essentially only Safari at this point so I'm not planning on a polyfill (Autoprefixer would take care of this anyway).

Before After
userlist_before userlist_after

(This is currently affected by #2635. It still works but doesn't look as good, so test without html-minifier-loader if you want to get the expected look)
This PR now fixes #2635.

This happens to fix #909.

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 13, 2018
@astorije astorije added this to the 3.0.0 milestone Jul 13, 2018
@astorije astorije requested a review from xPaw July 13, 2018 04:36
@xPaw
Copy link
Member

xPaw commented Jul 13, 2018

Looks fine. Should we perhaps put a border under the search bar?

And perhaps the input left padding should match so it doesn't look as misaligned?

@richrd
Copy link
Member

richrd commented Jul 14, 2018

Would it be enough if the input had a bottom border? Also would a border color CSS var make any sense? It would be nice to have IMO, but not in this PR.

@astorije
Copy link
Member Author

Looks fine. Should we perhaps put a border under the search bar?

@xPaw, it doesn't look like it because 💩 GIF, but the search has a different background (gray). It appears white on the GIF.

Otherwise I'm not touching the search field in this PR, it will be for a later PR, there are other things that need to be done on the search field anyway.

@xPaw
Copy link
Member

xPaw commented Jul 18, 2018

Does this PR fix #909 then?

@xPaw
Copy link
Member

xPaw commented Jul 18, 2018

So the sticky menu hides active user when scrolling with keyboard arrows (scroll down a little, then arrow up).

@astorije
Copy link
Member Author

Does this PR fix #909 then?

Yes! Thanks for noticing.

astorije and others added 3 commits July 21, 2018 02:25
…d improve constrast accessibility

#767676 is the lightest gray that passes AA level of WCAG contrast recommendations
@astorije
Copy link
Member Author

So the sticky menu hides active user when scrolling with keyboard arrows (scroll down a little, then arrow up).

Good catch. It looks like there is not solution to have position: sticky and scrollIntoView() to play well together.

However, this is resolved by changing to block: "center" here. What do you think about this?
It has minimal impact but, more importantly, as I was playing with this, I actually think it is nicer to use. When you press up/down, you know what's coming before/after rather than blindly going for the next user/channel. I do think it's a UX improvement actually.
Would you be okay with this change?

@xPaw
Copy link
Member

xPaw commented Jul 21, 2018

Does that option work in Firefox esr? We had problems with it.

Or Safari for that matter. https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView#Browser_compatibility

@astorije
Copy link
Member Author

Just tested and it works in Firefox 60. I believe the issues were with the "smooth" behavior, not this. Note that I don't have ESR but I believe ESR is 60, no?

On Safari, it works exactly the same with center or end, which means... it doesn't really work. Both act like start.

So yeah, I think we should use center. 👍 / 👎 ?

@@ -135,25 +128,6 @@
color: #b7c5d1;
Copy link
Member

Choose a reason for hiding this comment

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

This and #chat table.channel-list td also use this color, are we not able to use var here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done a couple things in 8c09be9, let me know what you think:

  • For unhandled messages, I moved the override to main styling and used muted color (I also applied it to the icons for MOTD and CTCP). I removed color for #chat .from which should always be set for specific things: colored or uncolored nicknames, icons, etc.
  • Removed color overrides for the channel list. There was no real reason to override the main color so now channel list table looks the same as ignored/ban user tables. I also removed the override on channel names in this table because I didn't think it brings anything to do that, and it's now more consistent with inline channels in chat content, and would keep it consistent if at some point we decided to style inline channels.

I didn't touch #windows .window h2 because that's something I'd want to do in the main style without resorting to this specific variable (so have a new variable for headings for example) and that's way outside of what I was trying to achieve here, I'll deal with this later if that works for you.

- This applies to channel list and user list sidebars
- This avoids having a selected item behind a sticky header
- This provides better UX by starting the scroll before the bottom (or top) is reached, giving a preview of what comes next (or before)
@astorije astorije merged commit cc4ab38 into master Jul 25, 2018
@astorije astorije deleted the astorije/sticky-userlist branch July 25, 2018 04:54
@dgw
Copy link
Contributor

dgw commented Jul 25, 2018

This happens to fix #909.

Works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
4 participants