Styling tweaks and a very stupid bugfix #8
Conversation
✅ Deploy Preview for enn-graphiql ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| @@ -204,5 +216,18 @@ button.graphiql-execute-button { | |||
| } | |||
|
|
|||
| .graphiql-explorer-actions { | |||
| display: none; | |||
| display: none !important; | |||
There was a problem hiding this comment.
There are inline styles there from Explorer plugin.
| width: '100%', | ||
| }} | ||
| className={props.className} | ||
| > | ||
| <SavedQuerySelect | ||
| queries={[]} |
There was a problem hiding this comment.
I still can't get to terms with the fact that I committed it.
| textIndent: '0.5em', | ||
| }, | ||
| }} | ||
| /> | ||
| {isCurrentDefault && <DefaultQueryChip />} | ||
| <DefaultQueryChip visible={!!isCurrentDefault} /> |
There was a problem hiding this comment.
This feels a bit odd, but I want to ensure that the Input doesn't jump when we switch to a default query.
| </Flex> | ||
| <Dropdown.Button asChild disabled={props.queries.length === 0}> | ||
| <Button variant="tertiary" sx={{ '> button': { px: Spacing['8px'] } }}> | ||
| <Button variant="tertiary" sx={{ '> button': { width: '48px', height: '48px', px: 0 }, flexShrink: 0 }}> |
There was a problem hiding this comment.
@benface This seems like a components problem to me. So, I'd like to have square icon buttons, evenly sized to match other content. How do you feel about
<Button variant="tertiary" width="48px" height="48px" sx={{ flexShrink: 0 }} />
Then that px... maybe we could check if the only child is an Icon?
There was a problem hiding this comment.
It's already supposed to work this way @hasparus. The following should result in a 48 x 48 button:
<Flex.Row>
<Button.Tertiary>
<Icon.CaretDown sx={{ transform: 'translateY(-4px)', opacity: props.queries.length ? 1 : 0 }} />
</Button.Tertiary>
</Flex.Row>Doesn't it? If it doesn't it's a bug.
There was a problem hiding this comment.
I've used '> button' selector two times.
Once for the queries select arrow trigger
<Dropdown.Button asChild disabled={props.queries.length === 0}>
<Button variant="tertiary">
<Icon.CaretDown sx={{ transform: 'translateY(-4px)', opacity: props.queries.length ? 1 : 0 }} />
</Button>
</Dropdown.Button>
and once for the actions menu three dots trigger
<Dropdown.Button asChild>
<Button size="medium" variant="tertiary">
<Icon.Options title="Open actions menu" sx={{ display: 'flex' }} />
</Button>
</Dropdown.Button> 
so both my icon buttons.
It seems that isIconOnly in the Button code is false.
Quick investigation led me to this log:
It starts working as intended if I remove the sx prop from the <Icon /> element, but then my icon is off by few pixels.
The reason behind this bug is that we're checking if something is an Icon by checking if Object.values(Icon).includes(element.type), and it doesn't work with Emotion wrapper elements.
It will also not handle any custom icon that's not included in the design system.
Tricky problem 🤔
What do you think about the following?
We currently allow more than one icon in Button children... if we didn't need it we could get rid of getChildrenChunks and use an API like this
<Button icon={<Icon.ThatLandsToTheRightOfChildren />}}>
Optional child
</Button>
<Button icon={<Icon.ThatLandsToTheRightOfChildren />}} iconPosition="left">
Optional child
</Button>and we wouldn't need to check Object.values(Icon) to know if something is an icon, because we'd have it explicitly given in icon prop.
There was a problem hiding this comment.
@hasparus – Sorry for the delay in my response, I kept this comment in my inbox because I wanted to give it the attention it deserved.
That's an unfortunate problem. :( I didn't realize that adding the sx prop to Icon would break this.
My reasoning for the icon syntax (passing it as a child instead of a prop) is not only for flexibility but also for DX. It makes the syntax of an icon-only button more consistent with that of a text-only button (e.g. <Button>Copy</Button> => <Button><Icon.Copy /></Button>, instead of <Button icon={<Icon.Copy />} />). It also makes the action of adding an icon to a text button (either on the left or on the right of the text) or changing the icon's position afterwards "feel" more natural and elegant to me. The way I see it, text and icon(s) are semantically the same in this context: they form the button's "label", so it makes sense to group them together. I realize all of this is kinda subjective and not a strong argument, especially since it requires some "magic" to make it work, and that magic sometimes breaks.
So, I'm open to changing this, but it would be a breaking change so first I would like to ask you if you can think of any way to still recognize icons when they've been wrapped by Emotion – maybe by detecting the class name gds-atoms-icon or something like that? That wouldn't fix the issue with "custom icons that are not included in the design system", but I'm not convinced we need to support that. If we do, we could export a Button.Icon component that would also be detected as an icon and could be used to wrap any element/component:
<Button>
<Button.Icon><CustomIconComponent /></Button.Icon>
Button Label
</Button>Let me know what you think, and thank you!
No description provided.