-
Notifications
You must be signed in to change notification settings - Fork 156
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
Style and UX fixes for push UI #589
Conversation
I'm going to leave the selected connections count for another issue/PR another day, this is ready for secondary review and merge. |
@helen Tested this out and this looks really good to me, definitely better then what we currently have. Did have a few comments though: For me, the Clear link doesn't line up with the Selected Connections text: This is more just my opinion but if you click the Select All button, wondering if we should disable that button, as clicking it again won't do anything at that point. At the moment, the button looks the same in both states: |
@dkotter Which browser are you using, and is this in the admin or on the front end? I've been checking in both Firefox and Chrome on Mac, not sure if maybe there are other factors that could be impacting this as well, since I noticed that themes on the front end can impact the CSS here. I notice your buttons are tall as well. As for disabling the Select All button, I am not opposed to it, I just don't know if it will end up being distracting turning on and off as you filter/search for connections? It doesn't seem harmful to leave it just not doing anything, but I can certainly try it and see how it feels locally. |
Changing the vertical alignment led to some other issues for me but removing the min-height seems to have worked out for me in the admin, so I pushed a change for that. One more look and then let's merge? |
👍 That fixed it, looks good for me now |
Description of the Change
Polishing style and UX bits for the push UI after adding
Select All
button.I currently have 3 things left on my list of nice-to-have items for this PR:
Add some kind of subtle indicator that a list can be scrolled, this tripped me up a few times. ExampleCan't use this method due to background colors, haven't found anything else, skipping for nowBefore
After
Alternate Designs
n/a
Benefits
A UI that's easier to understand.
Possible Drawbacks
Not sure.
Verification Process
Local testing with various states (search, selection, distributed, clearing). I also tested keyboard nav which looks fine to me.
Checklist:
Applicable Issues
Fixes #577
Changelog Entry
Needs.