-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reskin
| sidebar
#1605
Reskin
| sidebar
#1605
Conversation
✅ Deploy Preview for fractal-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Have bunch of comments, most of them are minor/optional. Leaving them just assuming there will be more changes and thus - another round of review. So feel free to make changes or ignore them - we can tackle these later for sure
@DarksightKellar @mudrila I've addressed your comments, and marked this PR ready for review |
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.
Lovely! Have few questions but feel free to ignore them if there's some context behind inability to style things through usual flow
onClick={closeDrawer} | ||
style={{ display: 'block', paddingTop: '0.75rem', paddingBottom: '0.75rem' }} |
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.
Can we omit using inline styles? Or it was too complex to get it styled right without inline styling?
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.
I might need your help with this one... The Link
component here is from react-router-dom
, not chakra-ui
, and so doesn't have any paddingTop
etc properties on it. I don't really know how else to add styles to this (I want to literally make the rendered anchor tag bigger) without either doing these inline styles, or defining a CSS class somewhere.
{...rest} | ||
target="_blank" | ||
rel="noreferrer noopener" | ||
style={{ display: 'block', paddingTop: '0.75rem', paddingBottom: '0.75rem' }} |
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.
tag should have display block by default, unless there's some weird global style override. Also, we can avoid using inline styling if we'll use plain Chakra UI link and will pass these styles as props
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.
Anchor tags seem to not be block
by default.
style={{ display: 'block', paddingTop: '0.75rem', paddingBottom: '0.75rem' }}
style={{ paddingTop: '0.75rem', paddingBottom: '0.75rem' }}
And can you help me with whatever it is you're suggesting to not use inline styles? I guess I'm not sure what approach you're suggesting.
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.
This is looking great!
I was having some weird issues with the component when I first pulled up the deploy preview (cursor glitching, needing to click to expand) but was unable to recreate once I reloaded and left the page open for a bit.
I don't see the hover, pressed, or active states active here yet, captured in this screen shot and recording.
Screen.Recording.2024-04-30.at.9.06.56.AM.mov
@nicolaus-sherrill sorry, where is that figma showing the hover and active states? |
7d518e1
to
6551748
Compare
Updates the left navigation sidebar
Some Screenshots: