feat: style & rename cell type outlines toggle #279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
@jimniels My thought for this one is lets go ahead with the rename and skip the Icon change. Keep the switch (for now). |
davidkircos
requested changes
Feb 28, 2023
src/ui/menus/TopBar/TopBar.tsx
Outdated
| backgroundSize: '18px', | ||
| }, | ||
| '&:before': { | ||
| backgroundImage: `url('data:image/svg+xml;utf8,<svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"><path fill="white" d="M6.2998 13.5L1.7998 9L6.2998 4.5L7.25605 5.45625L3.7123 9L7.25605 12.5438L6.2998 13.5ZM11.6998 13.5L10.7436 12.5438L14.2873 9L10.7436 5.45625L11.6998 4.5L16.1998 9L11.6998 13.5Z" /></svg>')`, |
There was a problem hiding this comment.
It seems like the icons could be a little bit smaller.
Also with the primary blue the switch really draws attention. Could you maybe do an outlined button with the icon?
There was a problem hiding this comment.
Just pushed up a change that inverts the color so it's not so attention-grabbing. Here's what it looks like, what do you think?
davidkircos
approved these changes
Mar 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Style the toggle in the upper right to make it hint more at its function (rather than a generic, label-less toggle).
Also, once #277 gets merged, the function of this toggle is less about "cell type" outlines and more about the outlines behind a specific kind of cell, e.g. code/formulas, so my suggestion is we actually rename it to be "Show code cell outlines".
Labels updated to "Show code cell outlines" in respective parts of app:
History
First iteration tried an icon that toggled its appearance:
Design notes:
I tried a couple different design ideas for this as a toggle
But ultimately felt that an icon in the same size/shape as the others in the toolbar was most effective.
For the on/off mode, rather than a fill or color change, I went with a dashed line. I tried a version where we color the "on" state, but it didn't quite feel appropriate and in line with how we use that color elsewhere in the app (which is a primary color and not necessarily a stateful color — though it is that way on the switches). I'm on the fence on this, but the current code keeps the icon the same color regardless of state (and only changes the shape).