-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ButtonGroup] Add theme to ButtonGroup and move CSS to Button #2441
Conversation
@@ -53,13 +53,13 @@ $partial-button-filled-pressed-box-shadow: inset 0 0 0 0 transparent, | |||
transition-duration: duration(fast); | |||
background: linear-gradient(to bottom, $color-light, $color-light); | |||
border-color: $color-dark; | |||
box-shadow: $partial-button-filled-pressed-box-shadow$color-dark; |
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.
Not part of the changed but noticed this while editing the file.
src/components/Button/Button.scss
Outdated
@@ -71,6 +71,38 @@ $partial-button-filled-pressed-box-shadow: inset 0 0 0 0 transparent, | |||
} | |||
} | |||
|
|||
.globalTheming.inSegmentedGroup, |
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.
Once we remove the .globalTheming these can be safely removed as well. Specificity issue.
src/components/Button/Button.tsx
Outdated
@@ -135,6 +139,27 @@ export function Button({ | |||
|
|||
const i18n = useI18n(); | |||
|
|||
const buttonGroupContext = useButtonGroupContext(); |
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.
Styling logic of the button itself now moved to Button.
153f21e
to
33a878c
Compare
@@ -1,4 +1,4 @@ | |||
import React from 'react'; | |||
import React, {useState} from 'react'; |
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.
took the opportunity to convert Item to a functional component.
1ad62cd
to
d13d87d
Compare
src/components/Portal/Portal.tsx
Outdated
@@ -67,8 +68,14 @@ export class Portal extends React.PureComponent<PortalProps, State> { | |||
} | |||
|
|||
render() { | |||
const childrenMarkup = ( | |||
<ButtonGroupContext.Provider value={undefined}> |
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.
@BPScott I was thinking this could be a valid option. I debated adding this to popover because additional buttons would always be inside a popover, however, new overlays could come at play one day and this will guard against these as well.
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 trick! I'll forget all about it in a few weeks so it'd be worth adding a comment about why this is here
15d8694
to
e7e3367
Compare
src/components/Button/Button.tsx
Outdated
@@ -124,6 +126,7 @@ export function Button({ | |||
}: ButtonProps) { | |||
const {unstableGlobalTheming = false} = useFeatures(); | |||
const hasGivenDeprecationWarning = useRef(false); | |||
const id = useUniqueId('Button', idProp); |
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.
Having to add an ID to every button feels rather heavy handed, I wonder if there is a way we can avoid this (or if not, can we make the id needed by the context be distinct from the id attribute that gets added to the DOM).
Perhaps instead of having the Buttons be responsible for adding themselves to the context, instead ButtonGroup could trawl through its children to collect the Button elements and adjust their 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.
What do you think of just using the Id internally and not modifying the DOM?
Having ButtonGroup look at its children and add props I think could be done by recursively looping through the children and adding a prop to the Button by using cloneElement. Is that what you have in mind? Doing that would limit our implementation to Buttons. Having Buttons register themselves means this can be added to any component using the registration hook. For instance, the first button in BulkAction isn't a Button per se, but with this approach, we can still make use to the group.
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'd be on board with using the id internally, I think that pollutes the DOM a little less.
As an aside how many renders does this approach trigger? Do we pay a price for rendering the button group, then when rendering the button updating the context (does that cause another render)
Having ButtonGroup look at its children and add props I think could be done by recursively looping through the children and adding a prop to the Button by using cloneElement. Is that what you have in mind? Doing that would limit our implementation to Buttons. Having Buttons register themselves means this can be added to any component using the registration hook.
Something like that yeah, though I'm not sure about its performance characteristics so I don't know if it's a good idea or not. We're changing the registration point from within the child element (e.g. Button) to the container (i.e. ButtonGroup) - we can make that say "look for Buttons and Popovers". We wouldn't be able to account for any component (e.g. something that exists in web) but being able to open that up to ANYTHING can be a child of a ButtonGroup feels a bit too open-ended to me at the moment (though I've not thought about this in depth)
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.
As an aside how many renders does this approach trigger? Do we pay a price for rendering the button group, then when rendering the button updating the context (does that cause another render)
That is a bit of a concern that it renders every time a button renders but the render is negligible I would say because buttons are small and there won't be that many in a group.
src/components/Portal/Portal.tsx
Outdated
@@ -67,8 +68,14 @@ export class Portal extends React.PureComponent<PortalProps, State> { | |||
} | |||
|
|||
render() { | |||
const childrenMarkup = ( | |||
<ButtonGroupContext.Provider value={undefined}> |
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 trick! I'll forget all about it in a few weeks so it'd be worth adding a comment about why this is here
); | ||
|
||
return ( | ||
<div className={className} onFocus={setFocused} onBlur={setFocused}> |
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.
Will this potentially cause oddness if you try and focus something that is already focussed, or blur something that is already blured? (e.g. if it's already focused causing it to focus again will cause it to toggle to the unfocused state instead of remaining focused)
useForcableToggle()
and its options for "always set to true" and "always set to false" would be better here.
e7e3367
to
cff14e1
Compare
cff14e1
to
d03c6ca
Compare
d03c6ca
to
7ff1a54
Compare
@chloerice I provide the playground above to confirm that the split button can be done: |
7ff1a54
to
f0d929b
Compare
Going with Ben's suggestion here and simply moving the selectors to Button using data-attributes. Using context gets us where we want but the only real advantage is knowing which is the first and last button at the cost of additional renders. This does allow us to use the data attributes to style whatever component we want to style with round corners. Thanks Ben! |
6da3e8f
to
fed5e46
Compare
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 works great, Dan!
🎩 Tophatted with this updated playground code:
import React, {useState, useCallback} from 'react';
import {CaretDownMinor} from '@shopify/polaris-icons';
import {
Page,
Select,
FormLayout,
TextField,
ButtonGroup,
Button,
Popover,
ActionList,
Card,
Layout,
} from '../src';
export function Playground() {
return (
<Page title="Playground">
<Layout>
<Layout.Section>
<Card title="New button group implementation supports grouping popover buttons">
<Card.Section title="Segmented button group with disclosure buttons">
<ButtonGroup segmented>
<Button>Bold</Button>
<Button>Italic</Button>
<Button>Underline</Button>
<PopoverWithActionListExample />
<PopoverFormExample />
</ButtonGroup>
</Card.Section>
<Card.Section title="Split button">
<SplitButtonExample />
</Card.Section>
<Card.Section title="Basic button group">
<ButtonGroup>
<Button>Bold</Button>
<Button>Italic</Button>
<Button>Underline</Button>
<PopoverWithActionListExample />
<PopoverFormExample />
</ButtonGroup>
</Card.Section>
<Card.Section title="Inventory quantity input">
<InventoryQuantityInput />
</Card.Section>
</Card>
</Layout.Section>
<Layout.Section secondary>
<Card title="New button group implementation supports grouping popover buttons">
<Card.Section title="Segmented button group with disclosure buttons">
<ButtonGroup segmented>
<Button>Bold</Button>
<Button>Italic</Button>
<Button>Underline</Button>
<PopoverWithActionListExample />
<PopoverFormExample />
</ButtonGroup>
</Card.Section>
<Card.Section title="Split button">
<SplitButtonExample />
</Card.Section>
<Card.Section title="Basic button group">
<ButtonGroup>
<Button>Bold</Button>
<Button>Italic</Button>
<Button>Underline</Button>
<PopoverWithActionListExample />
<PopoverFormExample />
</ButtonGroup>
</Card.Section>
<Card.Section title="Inventory quantity input">
<InventoryQuantityInput />
</Card.Section>
</Card>
</Layout.Section>
</Layout>
</Page>
);
}
function PopoverWithActionListExample() {
const [popoverActive, setPopoverActive] = useState(false);
const togglePopoverActive = useCallback(
() => setPopoverActive((popoverActive) => !popoverActive),
[],
);
const activator = (
<Button onClick={togglePopoverActive} disclosure>
More actions
</Button>
);
return (
<Popover
active={popoverActive}
activator={activator}
onClose={togglePopoverActive}
>
<ActionList items={[{content: 'Import'}, {content: 'Export'}]} />
</Popover>
);
}
function PopoverFormExample() {
const [popoverActive, setPopoverActive] = useState(false);
const [tagValue, setTagValue] = useState('');
const handleTagValueChange = useCallback((value) => setTagValue(value), []);
const togglePopoverActive = useCallback(
() => setPopoverActive((popoverActive) => !popoverActive),
[],
);
const activator = (
<Button onClick={togglePopoverActive} disclosure>
Filter
</Button>
);
return (
<Popover
active={popoverActive}
activator={activator}
onClose={togglePopoverActive}
sectioned
>
<FormLayout>
<Select
value="Tagged with"
label="Show all customers where:"
options={['Tagged with']}
onChange={() => {}}
/>
<TextField
label="Tags"
value={tagValue}
onChange={handleTagValueChange}
/>
<Button size="slim">Add filter</Button>
</FormLayout>
</Popover>
);
}
function SplitButtonExample() {
const [popoverActive, setPopoverActive] = useState(false);
const togglePopoverActive = useCallback(
() => setPopoverActive((popoverActive) => !popoverActive),
[],
);
const activator = (
<Button
accessibilityLabel="More save actions"
primary
onClick={togglePopoverActive}
icon={CaretDownMinor}
/>
);
return (
<ButtonGroup segmented>
<Button primary>Save</Button>
<Popover
active={popoverActive}
activator={activator}
onClose={togglePopoverActive}
preferredAlignment="right"
>
<ActionList
items={[{content: 'Save as draft order'}]}
onActionAnyItem={togglePopoverActive}
/>
</Popover>
</ButtonGroup>
);
}
function InventoryQuantityInput() {
const [currentOperation, setCurrentOperation] = useState('add');
const togglePressed = useCallback(
(operation) => () => {
setCurrentOperation(operation);
},
[],
);
const addActive = currentOperation === 'add';
const toggleButtons = (
<ButtonGroup segmented>
<Button onClick={togglePressed('add')} pressed={addActive}>
Add
</Button>
<Button onClick={togglePressed('set')} pressed={!addActive}>
Set
</Button>
</ButtonGroup>
);
const primaryButton = (
<Button disabled primary submit>
Save
</Button>
);
const inputLabel = addActive ? 'Add' : 'Set';
return (
<TextField
type="number"
label={inputLabel}
labelHidden
connectedLeft={toggleButtons}
connectedRight={primaryButton}
onChange={() => {}}
/>
);
}
One thing I noticed is that segmented button groups break in small spaces, but I think that's an issue with ButtonGroup
that pre-exists these changes 🤔
When I tophatted in web
, there's a few places that button groups break because they nest the ToggleButton
from @shopify/polaris-next
, which doesn't have this new CSS that makes this work for Button
. But the new pressed
boolean prop on the Button
does the same thing that ToggleButton
does, so we'd just want to either add the same CSS to ToggleButton
in @shopify/polaris-next
as these changes add to Button
or even better just open a PR to update the QuantityInput
in web
to use Button
with pressed
.
I also notice that the theming breaks, but I can't tell if it's because button theming is incomplete 🤔
I'll update #2329 to just be adding a split button example to the Button
docs using the ButtonGroup
😀
fed5e46
to
09000bd
Compare
Thank you for such a thorough 🎩@chloerice. That playground 😍
Are you referring to the icon on primary or was there something else? The wrapping does occur today, so this is not solved by this PR :( I also started removing the ToggleButton from |
version "1.7.8" | ||
resolved "https://registry.yarnpkg.com/@shopify/react-testing/-/react-testing-1.7.8.tgz#64b79d8b1cc4791d8e77e3ad5c06dfb7380c88b7" | ||
integrity sha512-P4CNcYPjX6OKMe4f6LfyTM0UWlQqinBSoFIWPrWRwAI0qnwKRYnP81WdUANpmeY01SxAIp/WwjKTgpg3sdTnoQ== | ||
"@shopify/react-testing@^1.8.0": |
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.
yarn.lock wasn't committed with #2465
🎉
Yes, the icon on primary, but also the negative margin on segmented button groups seems to disappear 🤔 |
I've fixed the icon color, but the gap is part of the new design. Which brings up a good point about the split button. I think it still makes sense but maybe we should check with @jessebc ? |
.Item { | ||
// stylelint-disable-next-line selector-max-class, selector-max-specificity | ||
&:not(:first-child) { | ||
margin-left: spacing(extra-tight); |
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 like we need to add a new variable at the top for this margin value like $segmented-spacing: rem(2px);
since spacing(extra-tight)
is rem(4px)
.
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.
Could have sworn this was 4px. I changed it now and was able to remove the .'globalTheming' class.
Ooohhh okay! Hmmm, I might need to add a |
Now with the smaller gap, I think it looks pretty good. Essentially now the only difference with segmented and non-segmented is that non-segmented keep all their round corners. |
533186e
to
5185a0a
Compare
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 good. Are we sure we want buttons in a non-connected group to be that close?
289255e
to
337cccd
Compare
337cccd
to
12714f4
Compare
You're right @tmlayton. I looked and this is used in card footers, and such. Removed the adjustments on the regular group. Once this is green I'll ship and get an update in a subsequent PR if need be, so I can continue with my next PR which depends on this one. @jessebc how is this looking? I kept the same spacing as today on the |
🎉 |
This is looking awesome @dleroux, sorry I didn't get around to reviewing before you merged. What do we think about the focus state for these? Should the ring match the shape of the underlying element? I think so.
|
I think this is looking very nice. My only question is around the split button. I feel like we should either do a 1px gap on that button, or potentially add a darker line as a visual break instead of the gap.
|
@danrosenthal Yes it should match but @tmlayton mentioned we might want to hold off and review @jessebc maybe instead of using ButtonGroup for this we might just have to stick with @chloerice solution of adding a prop to Button. Do you think there'll be other cases where we'd want a smaller gap? |
WHY are these changes introduced?
addresses part of: https://github.com/Shopify/polaris-ux/issues/382
and also
fixes: #1240
WHAT is this pull request doing?
ButtonGroup
(essentially margins).How to 🎩
When this gets into a release, this PR in web will need to be shipped at the same time.
Copy-paste this code in
playground/Playground.tsx
:Copy-paste this code for the split action button:
🎩 checklist
README.md
with documentation changes