Conversation
| /* this padding should be safe as button group is always end aligned */ | ||
| padding-inline-start: var(--spectrum-dialog-gap-size); | ||
|
|
||
| &.spectrum-Dialog-buttonGroup--noFooter { |
There was a problem hiding this comment.
This messes up the width of the buttons when vertical. @snowystinger please halp
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
LGTM, still need to digest the useEffect stuff a bit
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Ok button width looks fine in Dialog for vertical stacked ButtonGroups
| This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. You may obtain a copy | ||
| of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
There was a problem hiding this comment.
I think this is supposed to stay?
| display: inline-flex; | ||
| /* Added this so it would wrap */ | ||
| flex-wrap: wrap; | ||
| align-items: flex-end; |
There was a problem hiding this comment.
should all button groups flex end? or should this be something from slots?
maybe flex end is a good default and if needed, people can do flex-start through a new class
| flex-shrink: 0; | ||
| } | ||
|
|
||
| > .spectrum-Rule--vertical { |
There was a problem hiding this comment.
spectrum rule shouldn't be in this file? nor should tool, action button, or button. They'll have different names than their counterparts in the the button css file and the divider css file
| } | ||
|
|
||
| > .spectrum-Rule--vertical { | ||
| block-size: auto; |
|
|
||
| .spectrum-Button + .spectrum-Button { | ||
| margin-left: var(--spectrum-buttongroup-button-gap-x); | ||
| margin-inline-start: var(--spectrum-buttongroup-button-gap-x); |
| useEffect(() => { | ||
| if (orientation !== 'vertical') { | ||
| if (scale !== lastScale) { | ||
| if (lastScale === 'large' && scale === 'medium') { | ||
| setHasOverflow(false); | ||
| } else { | ||
| checkForOverflow(); | ||
| } | ||
| } | ||
| lastScale = scale; | ||
| } | ||
| }, [setHasOverflow, checkForOverflow, scale, orientation]); |
There was a problem hiding this comment.
I think this will have some trouble if children are added/removed or if the locale changes or if any strings change
example where that might happen, confirm button in dialog is pressed, it performs an async action before closing, and while it does that, it displays a spinner and says wait while disabled. If the buttons were horizontal and the wait made it wrap, then we'd end up in a situation that doesn't look very good
There was a problem hiding this comment.
We may need to use a MutationObserver https://caniuse.com/#feat=mutationobserver because the internal change in the confirm button to display a wait spinner may not trigger anything here
There was a problem hiding this comment.
When a change occurs, we'll need to remove the vertical class, useLayoutEffect to see if there's overflow and quickly reapply the vertical class if there is overflow
There was a problem hiding this comment.
state shouldBeVertical, setShouldBeVertical = default false
useLayoutEffect:
setShouldBeVertical(checkForOverflow)
deps: shouldBeVertical
useEffect:
children updated?
setShouldBeVerticalFalse
deps: children
|
|
||
| useEffect(() => { | ||
| if (orientation !== 'vertical') { | ||
| // I think performance could be optimized here by creating a global, debounced hook for listening to resize |
There was a problem hiding this comment.
we should probably make a useDebouncedResize hook or something with a single listener in it, we can probably put that off for now though, right now it's buttongroup, tabs, breadcrumbs, none of which are likely to have a lot of instances on a page at once
| slots={{ | ||
| button: { | ||
| isDisabled, | ||
| UNSAFE_className: classNames(styles, 'spectrum-Button') |
There was a problem hiding this comment.
this classname should be renamed to spectrum-ButtonGroup-button
| if (Component === V2ButtonGroup) { | ||
| return render( | ||
| <Provider theme={theme}> | ||
| <V2ButtonGroup data-testid={buttonGroupId} {...props}> |
There was a problem hiding this comment.
are any tests testing this? i think we can get rid of the V2 parity, it's pretty different in implementation no?
| * The axis the ButtonGroup should align with. | ||
| * @default "horizontal" | ||
| */ | ||
| orientation?: Orientation, |
There was a problem hiding this comment.
Should it be mentioned that if vertical is used, no reflow will happen, it'll just always be vertical. But if horizontal is used, then reflow to vertical will happen?
There was a problem hiding this comment.
I think this should be documented in the docs, I'll say something like "See the docs for reflow behavior".
There was a problem hiding this comment.
will be easier to link to it from here
| } | ||
|
|
||
|
|
||
|
|
|
Build successful! 🎉 |
Co-Authored-By: Devon Govett <devongovett@gmail.com>
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
6 participants
No description provided.