Steplist component by dgrossen · Pull Request #5201 · adobe/react-spectrum

dgrossen · 2023-10-04T22:16:50Z

Reviving work on the StepList component and dusting off PR #2832

Fixed some bugs with rtl and the use of solid/dashed lines for vertical completed steps
Updated stories
Cleaned up dependencies

Still need to add tests and docs but thought I would get the ball rolling.

StepList Component Issue - #1012

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

rspbot · 2023-10-05T22:52:48Z

Build successful! 🎉

rspbot · 2023-10-05T23:09:12Z

Build successful! 🎉

snowystinger

In general it's looking really good. Thank you for picking this up.

I don't seem able to undo any step except the last one
steps:
default story, keyboard navigate to the steplist and then to the last item
keyboard navigate back to the first item, only the last step is cleared out

Rest of my comments I think could be candidates for followup:

would be good to have less stories if they are covered by the controls, and instead add the more edge case stories, such as ones with steps that have long names which wrap or cause the steplist to take 100% of the width, or show them in a dialog, or maybe one with controlled selection that updates in response to events, or a disabled key in the middle but still have some enabled toward the far end. or starting with a disabled key. it'd also be good to have one with the keyboard activation set to manual

I have a feeling that we'll want/need to support links in steplists, we'll need an example of that to make sure it works
what I mean by that is specifically https://react-spectrum.adobe.com/react-spectrum/Breadcrumbs.html#links

snowystinger · 2023-10-06T04:47:07Z

+        )}>
+        {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
+        <a className={classNames(styles, 'spectrum-Steplist-link')} >
+          <VisuallyHidden {...stepStateProps}>{stepStateText}</VisuallyHidden>


I'm not hearing "Completed" ever read out on MacOS VO

snowystinger · 2023-10-06T04:48:35Z

+            {item.rendered}
+          </span>
+          <span className={classNames(styles, 'spectrum-Steplist-marker-focus')}>
+            <span className={classNames(styles, 'spectrum-Steplist-marker')}>{(item.index || 0) + 1}</span>


we should internationalize this number using a number formatter

I also feel like the announcement should prioritize the step number followed by the title
so instead of it reading "Dog 2", it should say something like "Step 2, Dog"
or, it should say "Dog, step 2"

snowystinger · 2023-10-06T05:15:39Z

+  return {
+    ...state,
+    setSelectedKey,
+    setLastCompletedStep,


where is this used? can it be controlled through props selectedKey?

dgrossen · 2023-10-13T23:33:51Z

@snowystinger please give it another review when you get a chance. I addressed most of the feedback here and several of the items from the testing bash. I also cleaned up some things. Opted to use context instead of passing the state as a prop to the items. I also got rid of the isNavigable stuff to align more with the #1012 spec. It should also follow the XD spec more closely also

rspbot · 2023-10-13T23:46:46Z

Build successful! 🎉

rspbot · 2023-10-16T17:13:34Z

Build successful! 🎉

snowystinger · 2023-10-17T03:18:34Z

Some behavioral things I noticed:
Up and Down arrow keys will still navigate
Can't click on any of the steps anymore really in any of the stories because there are so many disabled?
I don't know if most of the stories are missing an action for 'onAction', but I don't see onAction being fired in any of the stories
What is the Default story? it doesn't seem to do anything? I think it got mixed with the fully disabled story?

In the last story, when I press next, I can go to step 4, but then if I click on step < 4, I can't get back to step 4 without clicking the next button eventually

Unsure about this one, but when individual steps are disabled, they don't display their number, yet steps around them will. I think it looks funky, not sure I like it.

rspbot · 2023-10-17T23:44:11Z

Build successful! 🎉

dgrossen · 2023-10-17T23:54:50Z

Up and Down arrow keys will still navigate

Fixed

Can't click on any of the steps anymore really in any of the stories because there are so many disabled?

This is because I made changes to align more with the spec: #1012
Specifically, "Users can optionally click on completed steps to jump back to them, but advancing to the next step must be done through alternate means" and "By default, the last completed step advances automatically as the selected step increases". And I spoke with XD and they confirmed that expected behavior. Sorry for the confusion, the first story/implementation allowed any steps to be clicked on.

In the last story, when I press next, I can go to step 4, but then if I click on step < 4, I can't get back to step 4 without clicking the next button eventually

I improved the stories a bit to help clear things up. I tried to rename them and added a last completed step display to help communicate what's going on. The back button and next button are disabled when on the first and last step respectively.

I also added tests that hopefully describe the expected controlled and uncontrolled (sort of) states of the step list component.

Unsure about this one, but when individual steps are disabled, they don't display their number, yet steps around them will. I think it looks funky, not sure I like it.

I wasn't able to repro this one, but I wondered if this was related to the designs not having the numbers in incomplete steps for medium size, but the large and extra large incomplete steps do? https://spectrum-contributions.corp.adobe.com/page/steplist-beta/

rspbot · 2023-10-18T18:38:42Z

Build successful! 🎉

snowystinger

we should check all package jsons so we don't have extraneous dependencies, I did a spot check

rspbot · 2023-10-25T01:56:00Z

Build successful! 🎉

rspbot · 2023-10-25T16:44:44Z

Build successful! 🎉

rspbot · 2023-10-25T17:12:50Z

Build successful! 🎉

rspbot · 2023-10-25T17:43:34Z

Build successful! 🎉

rspbot · 2023-10-25T20:20:02Z

Build successful! 🎉

rspbot · 2023-10-25T20:59:46Z

Build successful! 🎉

reidbarber

I think the isEmphasized text color may fail contrast requirements, so we may want to revisit that design. Besides that, I think this looks ready for an alpha release (after the empty documentation pages get removed.)

LFDanLu · 2023-10-26T21:14:21Z

For followup: address HCM styling, especially for disabled steplist

LFDanLu · 2023-10-26T21:29:18Z

don't think we need this file

LFDanLu · 2023-10-26T21:31:33Z

+  "completed": "Completed: ",
+  "notCompleted": "Not completed: ",
+  "current": "Current: ",
+  "steplist": "Step List"


don't think we need this line since the aria hook is the one that needs this translation?

This is for localizing the VisuallyHidden component states of the step to the DOM:

<li> <a role="link" tabIndex="0"> <VisuallyHidden>Completed: </VisuallyHidden> Details </a> </li>

Right, but we don't need "steplist" if I'm not mistaken? Only the previous three lines are used:

react-spectrum/packages/@react-spectrum/steplist/src/StepListItem.tsx

Lines 55 to 61 in 060af14

if (isSelected) {

stepStateText = stringFormatter.format('current');

} else if (isCompleted) {

stepStateText = stringFormatter.format('completed');

} else {

stepStateText = stringFormatter.format('notCompleted');

}

rspbot · 2023-10-27T00:13:23Z

Build successful! 🎉

snowystinger

Tracking remaining work for the team here https://github.com/orgs/adobe/projects/19/views/4?pane=issue&itemId=42835801

rspbot · 2023-10-27T16:47:49Z

Build successful! 🎉

rspbot · 2023-10-27T16:47:51Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

LFDanLu

Happy for this to be merged as is for testing and for the rest to be followup

Puscasu Razvan and others added 5 commits February 10, 2022 19:42

add steplist component

e47c341

add steplist aria translations

25a7cf0

cleanup

aeb64fc

Merge branch 'steplist-component' into enhancement_1012-steplist

021d413

dust off PR 2832; update deps and stories

23fe059

snowystinger reviewed Oct 5, 2023

View reviewed changes

Comment thread packages/@react-spectrum/steplist/test/StepList.test.js Outdated

snowystinger reviewed Oct 5, 2023

View reviewed changes

Comment thread packages/@react-spectrum/steplist/test/StepList.test.js Outdated

Dan Grossen added 4 commits October 5, 2023 13:04

remove v2 tests

4981ef6

fix lint errors for steplist

6bc0d98

fix lint errors in steplist package.json files

55c26f8

add one working test for steplist

2241dad

revert useSelectableCollection change

a648989

snowystinger reviewed Oct 6, 2023

View reviewed changes

address PR feedback; align more with spec #1012

f76d004

dgrossen requested a review from snowystinger October 13, 2023 23:33

Merge branch 'main' into steplist-component

4044402

snowystinger reviewed Oct 13, 2023

View reviewed changes

cleanup stories and lingering console logs

af91ce1

Dan Grossen added 2 commits October 17, 2023 17:26

steplist - add tests, fix bugs, update stories

0d54e26

fix lint error

484fbf7

remove ts-ignore

55c7ea1

dgrossen commented Oct 24, 2023

View reviewed changes

Comment thread packages/@react-spectrum/steplist/test/StepList.test.tsx Outdated

fix step list tests; update aria step list attributes

4bf33d1

dgrossen commented Oct 25, 2023

View reviewed changes

Comment thread packages/@react-spectrum/steplist/test/StepList.test.tsx Outdated

snowystinger reviewed Oct 25, 2023

View reviewed changes

Comment thread packages/@react-spectrum/steplist/package.json Outdated

Comment thread packages/@react-spectrum/steplist/package.json Outdated

Comment thread packages/@react-spectrum/steplist/package.json Outdated

fix react 17 tests

f8ed19c

add findDefaultSelectedKey for Step List state

e36099b

allow tab to first key when not complete

cfecec0

Dan Grossen added 2 commits October 25, 2023 11:23

remove unnecessary deps for step list

bcde9ed

fix missing collections dependency

5a6e460

fix steplist story warning

8b32882

fix issue where steplist updating state while rendering

85d1690

reidbarber reviewed Oct 26, 2023

View reviewed changes

LFDanLu reviewed Oct 26, 2023

View reviewed changes

address PR feedback for docs and labeling

060af14

snowystinger approved these changes Oct 27, 2023

View reviewed changes

snowystinger mentioned this pull request Oct 27, 2023

WIP add steplist componente #2832

Closed

5 tasks

dgrossen and others added 2 commits October 27, 2023 07:36

Merge branch 'main' into steplist-component

4fa6078

Merge branch 'main' into steplist-component

8be6a4e

LFDanLu approved these changes Oct 27, 2023

View reviewed changes

LFDanLu merged commit d8438da into main Oct 27, 2023

LFDanLu deleted the steplist-component branch October 27, 2023 18:45

	if (isSelected) {
	stepStateText = stringFormatter.format('current');
	} else if (isCompleted) {
	stepStateText = stringFormatter.format('completed');
	} else {
	stepStateText = stringFormatter.format('notCompleted');
	}

Uh oh!

Conversation

dgrossen commented Oct 4, 2023

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

Uh oh!

Uh oh!

rspbot commented Oct 5, 2023

Uh oh!

rspbot commented Oct 5, 2023

Uh oh!

snowystinger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgrossen commented Oct 13, 2023

Uh oh!

rspbot commented Oct 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rspbot commented Oct 16, 2023

Uh oh!

snowystinger commented Oct 17, 2023

Uh oh!

rspbot commented Oct 17, 2023

Uh oh!

dgrossen commented Oct 17, 2023

Uh oh!

rspbot commented Oct 18, 2023

Uh oh!

Uh oh!

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

rspbot commented Oct 25, 2023

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

snowystinger left a comment •

edited

Loading

LFDanLu Oct 26, 2023 •

edited

Loading