-
Notifications
You must be signed in to change notification settings - Fork 11
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
Type, colour and spacing UI improvements #355
Conversation
This is 🔥 🔥 🔥 If you feel up to the task, I'd totally be open to us having two separate themes for light and dark. There are a fair few places where color doesn't transition well when going from dark->light or the other way round. E.g. The hover in dark mode I'd suggest we don't add such a saturated hue. Either way, this is a massive improvement! |
top: 0; | ||
left: 0; | ||
right: 0; | ||
`; | ||
|
||
const Item = styled.a<{ alignRight?: boolean }>` |
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.
Nice 👍
|
||
export default { | ||
basic: ( | ||
<Toolbar |
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.
Thanks for adding a fixture!
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.
Looks dope!
I'd be tempted to keep existing composition (providing children) rather than props for most presentational elements so we don't lose any flexibility (additional props, changing elements, ref access, etc).
Not blocking though! Maybe others have thoughts too 👍
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.
Awesome 🔥
This looks so much cleaner/more consistent
src/panel/components/Navigation.tsx
Outdated
const Logo = styled(Icon)` | ||
width: 32px; | ||
height: 19px; | ||
> * { |
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.
Given that it's only targeting descendents of <Logo />
it's probably fine but if we can it might be nice to restrict the selector slightly
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.
Good spot - have fixed the other instance of this also c339765
@@ -12,3 +12,13 @@ code, | |||
pre { | |||
font-family: "Roboto Mono", monospace; | |||
} | |||
|
|||
button { |
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 like this approach of resetting css, though if we do want to reset/remove default styles on a button might it be worth resetting styles for other HTML tags? (alternatively we could try and have some sort of reusable button component, though it looks like we have a bunch of different buttons with different styles currently)
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.
Yeah, good point. My preference is always to reset on a global level to avoid bloating component styles but I can see that the current approach is to unset on a per-component basis. If we went with a global reset for all HTML elements there'd be a good cleanup opportunity?
@andyrichardson any thoughts on this?
Thank you all for your comments! @kadikraman good spot on the navigation background - looks like that was only happening on the electron version which is why I missed it - will be sure to test on that next time! Fixed with this 46b9b7b @andyrichardson agree on your point on the composition of On the point of theming, as discussed I will look at that in a separate PR 😄 |
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.
Tested on React Native and looks great! 🎉 😍
The intention of these changes is to improve the usability of the devtools.
Changes include:
urql
brand purple to highlight interactive elements including active navigation itemsurql
logo with link to documentation<Toolbar />
component for sharing action style and logic between events and requests pages<Pane.Item />
and<Pane.Header />
components to reduce style duplication across pagesPlease check out the before/after screenshots below - happy to take any suggestions!
Apologies for the file change count... A lot of snapshots got updated 😬