-
Notifications
You must be signed in to change notification settings - Fork 79
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
Fix/tweak settings designs (#2537) #2561
base: develop
Are you sure you want to change the base?
Conversation
@jgaylor do you have any feedback on these screenshots? (this is mike's adaptation of the new mobile-style settings designs for desktop.) |
Thanks for sharing this. Here are some quick thoughts: Dark/light mode: Overall, it looks good except for the radio buttons and the visibility icon. For icons, we should probably use grayscale. For radios in dark mode, the colors should be reversed, but that would be temporary anyway based on my comments later... Panels: It's difficult to tell if they all have the same width since the screenshots are at various zoom levels, but they should if not. Additionally, we're mixing existing content layout with the new drawer, so there's no clear guide for how that should look. And design should layout this view in the new system. For example, the Notifications drawer needs a design that matches our new convention (action row with a checkmark). Holmes mentioned this will be on a branch soon, which will make it easier for me to review in more detail. |
Code looks good but we should fix the desktop tests. That should just require updating snapshots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. We can alter the length of the hidden invite link to fit the drawer now or later, doesn't matter to me.
This PR:
Pull Request Checklist
(Optional) Mobile checklist
Please ensure you completed the following checks if you did any changes to the mobile package: