WIP add steplist componente - #2832
Conversation
| "@react-aria/link": "^3.2.0", | ||
| "@react-aria/selection": "^3.7.1", | ||
| "@react-aria/tabs": "^3.1.0", | ||
| "@react-aria/utils": "3.4.1", |
There was a problem hiding this comment.
any reason for the fixed version ?
There was a problem hiding this comment.
your editor probably did it, it happens to me sometimes, it should be a range
| @@ -0,0 +1,67 @@ | |||
| { | |||
| "name": "@react-spectrum/steplist", | |||
There was a problem hiding this comment.
revisit deps, we don't need all 4 sure
| "@spectrum-icons/ui": "3.2.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@adobe/spectrum-css-temp": "^3.0.0-alpha.1" |
There was a problem hiding this comment.
bump version to alpha in pkg
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Took a very quick peek at a couple files, by no means an exhaustive review
| margin-right: -10px; | ||
| } | ||
| .spectrum-Steplist-chevron { | ||
| &.is-reversed { |
There was a problem hiding this comment.
should be able to make use of [dir=ltr] & [dir=rtl] instead of adding a class
| /* Focus */ | ||
| &:focus, | ||
| &.is-focused, | ||
| *:focus { |
There was a problem hiding this comment.
this selector terrifies me, what's going on or why do we need to select everything?
I think we only use the class now, shouldn't need :focus selectors
There was a problem hiding this comment.
yeah true, haven't revised the css yet
There was a problem hiding this comment.
It has been a while since I worked on this, but I'm pretty sure that I brought that over from spectrum-css directly without taking the time to investigate why it had so many different ways to select focused elements.
There was a problem hiding this comment.
Yeah, I'm pretty much assuming these are coming from spectrum-css, we have similar things all over the place that we're slowly cleaning up. Though if you're able to answer why there is a star selector, that'd be really helpful. Otherwise I'll investigate it at some point.
There was a problem hiding this comment.
i'll look over the css and clean it up a little 👍
| /* Override last-child bits */ | ||
| margin-right: 0; | ||
| margin-left: calc(calc(var(--spectrum-steplist-marker-hitArea) / 2) * -1); | ||
| grid-area: chevron; |
There was a problem hiding this comment.
if there is no chevron in vertical, let's rename the grid area to be more meaningful
| * The ref attached to the element representing the collection. | ||
| */ | ||
| ref: RefObject<HTMLElement>, | ||
| ref: RefObject<HTMLElement | HTMLOListElement>, |
There was a problem hiding this comment.
HTMLOListElement should be an HTMLElement, I don't think we need this 'or'?
| let [lastCompletedStep, setLastCompletedStep] = useControlledState<Key>(props.lastCompletedStep, props.defaultLastCompletedStep, props.onLastCompletedStepChange); | ||
| const {setSelectedKey: realSetSelectedKey, selectedKey, collection} = state; | ||
| const {prevKeyMap, indexMap} = useMemo(() => buildKeysMaps(collection), [collection]); | ||
| const disabledKeys = useMemo(() => new Set(props.disabledKeys), [props.disabledKeys]); |
There was a problem hiding this comment.
not sure if disabledKeys should fallow same approach as tabs breadcrumbs ... it feels weird to me that if step 3 is disabled user can still navigate to 4-5-6.... since i feel steps should be completed in sequence, i fallowed up with design on this one. If someone has more info on this pls feel free to share.
|
Closing for this #5201 |
4 participants
Closes
✅ Pull Request Checklist:
I took the great work @itaylor did on step list and did some of the changes that were discussed like:
ariahooks to fallow samepattern, like the rest of the components passingpropsstaterefto get the componentsDOMpropskeyboarddelegation foruseStepListcurrently it's the same like fortabsbut i will fallow withspectrumdesign to get the corectkeyboardnavigation behaviouruseSelectableCollectioninuseStepListaria-${props}out ofuseStepItemtoStepItemto fallow the same pattern with the rest of the componentsMy Plan:
stylingand make sure we are aligned withUXflexstoriesand add more examplesLet me know your thoughts