feat: Steps #2684

Revuj · 2024-09-09T10:11:49Z

Description

Related links, issue #, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Revuj · 2024-09-09T10:12:44Z

src/steps/styles.scss

+
+    > .step-details {
+      align-items: center;
+      font-size: awsui.$font-size-body-s;


font-size and color should not be set here. I'll remove this

Revuj · 2024-09-09T10:13:11Z

src/test-utils/dom/steps/index.ts

+import statusIconStyles from '../../../status-indicator/styles.selectors.js';
+import testUtilStyles from '../../../steps/test-classes/styles.selectors.js';
+
+export class StepWrapper extends ComponentWrapper {


This is WIP but any feedback regarding what selectors we should have is appreciated

cansuaa

As a side note, for type changes to be reflected in documentation don't forget to build the package and then update the documenter snapshot

cansuaa · 2024-09-09T11:45:14Z

src/steps/internal.tsx

+
+const InternalStep = ({ status, statusIconAriaLabel, header, details }: StepsProps.Step) => {
+  return (
+    <li className={clsx(styles['step-container'], testUtilStyles['step-container'])}>


You don't need separate classes. You can use the ones in styles. One minor thing, you also don't need to append step- to the beginning of each class name. Here you can simply use step and below status, header, etc.

cansuaa · 2024-09-09T12:10:36Z

src/test-utils/dom/steps/index.ts

+  }
+}
+export default class StepsWrapper extends ComponentWrapper {
+  static rootSelector: string = testUtilStyles.steps;


The same case with my comment above, you don't need the testUtilStyles, here you can use styles.root which is usually how it's defined for the other components as well.

cansuaa · 2024-09-09T12:45:46Z

src/steps/internal.tsx

+  return (
+    <li className={clsx(styles['step-container'], testUtilStyles['step-container'])}>
+      <div className={clsx(styles['step-status'], testUtilStyles['step-status'])}>
+        <StatusIndicator type={status} iconAriaLabel={statusIconAriaLabel} />


It seems that only icon is being used from status indicator? If we only want to get the icon then why not use the icon component?
Another question, do we expect the text to be the same color as the status icon? e.g. green when status is success?

If we only want to get the icon then why not use the icon component?

Good point!

Another question, do we expect the text to be the same color as the status icon? e.g. green when status is success?

That is really up to the consumer. There are use cases where consumers want to do that and others where they don't

Let's discuss switching it to icon with Kashi as well. If it's an icon, we could even expose the other available icon names and custom icons through url or svg but there could be a specific reason why status indicator was chosen (which could be the point I brought up whether we expect text to be green, red, etc.)

If we only want to get the icon then why not use the icon component?

I was looking into this and one issue is that the https://refresh.cloudscape.aws.dev/components/icon does not have Loading, so we would need to provide some custom logic for that. Because of that I would propose continuing with using the StatusIndicator. WDYT?

Hmm true that is lacking but we can easily switch it with a spinner. Though it'd mean some API changes and implementation changes. I'd say you leave it as is right now and then we discuss with Kashi

cansuaa · 2024-09-09T12:46:37Z

package.json

@@ -117,6 +117,7 @@
    "prettier": "^3.2.5",
    "react": "^16.14.0",
    "react-dom": "^16.14.0",
+    "react-markdown": "^8.0.7",


Do we really need this?

Not really! I mostly added it because in Q we are gonna be using this so I wanted to validate it works as expected but I can replace it by something else and avoid adding a devDependency here

cansuaa · 2024-09-09T12:55:44Z

src/test-utils/dom/steps/index.ts

+  findStatusIndicator(): ElementWrapper | null {
+    return this.findByClassName(statusIconStyles.icon);
+  }


I'm not sure how helpful this would be given it only is an icon

Isn't it useful so that consumers can test that their expected status is there?

Not really, this would only return an icon element which is alone not enough to communicate the status, besides it won't really indicate the status unless they depend on the class name which is not recommended. If you check status indicator, it also doesn't have a selector for the icon.

codecov · 2024-09-10T10:53:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.16%. Comparing base (6c9f530) to head (e599846).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2684   +/-   ##
=======================================
  Coverage   96.15%   96.16%           
=======================================
  Files         756      759    +3     
  Lines       21303    21329   +26     
  Branches     6895     6896    +1     
=======================================
+ Hits        20484    20510   +26     
  Misses        811      811           
  Partials        8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cansuaa · 2024-09-13T08:12:32Z

src/steps/internal.tsx

+  return (
+    <div
+      {...props}
+      className={clsx(styles.root, styles.steps, props.className)}


props.className?

A minor point is that you don't need two separate classes root and steps, only root is enough

cansuaa · 2024-09-13T08:44:17Z

src/steps/__tests__/steps.test.tsx

+    expect(wrapper.findItems()[3]!.findDetails()).toBeNull();
+  });
+
+  test('renders correct status icons', () => {


This test isn't needed, you are testing if status indicator is putting the icon and the test can't even know if they are correct. Visual aspects like this are tested via visual/screenshot tests

cansuaa · 2024-09-13T08:47:11Z

src/steps/styles.scss

+    > :last-of-type > .connector {
+      display: none;
+    }


Could you check if screenreader is able to find this connector?

Confirmed that it is not able to find it :)

cansuaa · 2024-09-13T08:54:39Z

pages/steps/permutations-with-updates.page.tsx

Tbh I don't think this page is needed we know the component will be updated when customer updates the steps. It also requires interactivity which won't do much for screenshot tests. I'd say the same thing for the interactive permutations.

So I should just leave the regular permutations.page right?

cansuaa · 2024-09-13T08:59:14Z

pages/steps/permutations-with-updates.page.tsx

+
+  return (
+    <ScreenshotArea disableAnimations={false}>
+      <article aria-live="polite">


Can you confirm this announces every change in the steps?

Confirmed :)

jperals · 2024-09-13T10:38:59Z

pages/steps/styles.scss

What is this file for?

It contains the styles being used in the permutations page. I can remove it and make it inline css if you'd prefer

gethinwebster · 2024-09-23T11:48:45Z

src/steps/interfaces.ts

+   * Each step definition has the following properties:
+   *  * `status` (string) - Status of the step corresponding to a status indicator.
+   *  * `statusIconAriaLabel` - (string) - (Optional) Text alternative for the status icon.
+   *  * `header` (react.ReactNode) - Area next to the status indicator of the step.


just (ReactNode)

Suggested change

* * `header` (react.ReactNode) - Area next to the status indicator of the step.

* * `header` (ReactNode) - Area next to the status indicator of the step.

I think the description of header could be improved too, maybe work with Kashi to align how it will be referred to in the usage guidelines

gethinwebster · 2024-09-23T11:50:44Z

src/steps/internal.tsx

+
+export const InternalSteps = ({ steps, __internalRootRef, ...props }: InternalStepsProps) => {
+  return (
+    <div


shouldn't these aria-* properties be on the semantic element, i.e. the ol?

jperals · 2024-09-26T10:30:17Z

pages/steps/with-updates.page.tsx

+    successfulStepsInteractive,
+  ];
+
+  const [stepIndex2, setStepIndex2] = useState(0);


This can probably be refactored with a component that manages its own internal state independently, instead of managing 4 almost-identical states in a single component.

That is true, but is it worth it at this point? :) It's just an example page

Let's call this non-blocking but a suggestion. Always good to keep things simple for the next one who might work on it.

jperals · 2024-09-30T08:12:19Z

package-lock.json

@@ -6817,108 +6817,6 @@
      "integrity": "sha512-9rTIZ4ZjWwalCPiaY+kPiALLfOKgAz5CTi/Zb1L+qSZ8PH3zVo1T8JcgXIIqg1iM3pZ6hF+n9xO5r2jZ/SF+jg==",
      "dev": true
    },
-    "node_modules/devtools/node_modules/debug": {


Do you know why these changes in the package-lock.json popped up? If not, can we exclude them from the PR?

Excluded :)

jperals · 2024-09-30T16:53:26Z

src/steps/interfaces.ts

+   *
+   * Each step definition has the following properties:
+   *  * `status` (string) - Status of the step corresponding to a status indicator.
+   *  * `statusIconAriaLabel` - (string) - (Optional) Text alternative for the status icon.


"Text alternative" or "Alternative text"?

Rafa Varela and others added 2 commits August 30, 2024 17:06

add steps component and initial permutations

5f21361

add unit tests for steps component

f97fc6a

Revuj commented Sep 9, 2024

View reviewed changes

cansuaa reviewed Sep 9, 2024

View reviewed changes

cansuaa changed the title ~~Feat steps~~ feat: Steps Sep 10, 2024

Revuj and others added 3 commits September 10, 2024 11:00

add accessibility tests

643501e

Merge branch 'main' into feat-steps

50b5ba6

add steps component documentation

591be97

Revuj added 2 commits September 10, 2024 15:05

fix tests

5aded93

fix tests

4bbf553

Revuj had a problem deploying to dev-pages September 10, 2024 14:36 — with GitHub Actions Failure

cansuaa deployed to dev-pages September 10, 2024 14:48 — with GitHub Actions View deployment

update component design

b559980

cansuaa reviewed Sep 13, 2024

View reviewed changes

remove interactive permutations

e5634e5

Revuj marked this pull request as ready for review September 13, 2024 10:19

Revuj requested a review from a team as a code owner September 13, 2024 10:19

Revuj requested review from jperals and removed request for a team September 13, 2024 10:19

jperals reviewed Sep 13, 2024

View reviewed changes

remove permutations stylesheet

8ba6259

Revuj had a problem deploying to dev-pages September 18, 2024 08:10 — with GitHub Actions Failure

gethinwebster reviewed Sep 23, 2024

View reviewed changes

Revuj added 5 commits September 23, 2024 17:55

update steps interface description

b43c120

move a11y props to semantic element

c0cb5c7

add more steps permutations

9290084

Merge branch 'main' into feat-steps

f6aab80

Merge branch 'main' into feat-steps

632f69e

cansuaa deployed to dev-pages September 26, 2024 09:54 — with GitHub Actions View deployment

jperals reviewed Sep 26, 2024

View reviewed changes

Revuj and others added 3 commits September 26, 2024 12:55

Merge branch 'main' into feat-steps

a6ed61e

Merge branch 'main' into feat-steps

a95bae9

remove margin from ol

e3de7db

Revuj had a problem deploying to dev-pages September 26, 2024 12:56 — with GitHub Actions Failure

cansuaa deployed to dev-pages September 26, 2024 12:58 — with GitHub Actions View deployment

Merge branch 'main' into feat-steps

79cb69c

Revuj had a problem deploying to dev-pages September 27, 2024 08:40 — with GitHub Actions Failure

fix aria-live examples and add labels

e4a905f

Revuj force-pushed the feat-steps branch from 8e7c8d7 to e4a905f Compare September 27, 2024 09:29

Merge branch 'main' into feat-steps

968bafe

Revuj had a problem deploying to dev-pages September 27, 2024 09:38 — with GitHub Actions Failure

cansuaa deployed to dev-pages September 27, 2024 09:39 — with GitHub Actions View deployment

cansuaa requested a review from jperals September 27, 2024 13:23

jperals reviewed Sep 30, 2024

View reviewed changes

Revuj added 2 commits September 30, 2024 15:18

revert package-lock changes

20ad325

Merge branch 'main' into feat-steps

cea015e

Revuj had a problem deploying to dev-pages September 30, 2024 14:53 — with GitHub Actions Failure

cansuaa deployed to dev-pages September 30, 2024 15:58 — with GitHub Actions View deployment

jperals reviewed Sep 30, 2024

View reviewed changes

Revuj added 2 commits October 1, 2024 09:17

update statusIconAriaLabel description

cf8ffcc

Merge branch 'main' into feat-steps

e599846

jperals approved these changes Oct 1, 2024

View reviewed changes

Revuj had a problem deploying to dev-pages October 1, 2024 08:24 — with GitHub Actions Failure

cansuaa deployed to dev-pages October 1, 2024 08:27 — with GitHub Actions View deployment

cansuaa approved these changes Oct 1, 2024

View reviewed changes

cansuaa added this pull request to the merge queue Oct 1, 2024

Merged via the queue into cloudscape-design:main with commit 1686229 Oct 1, 2024
70 of 72 checks passed

	* * `header` (react.ReactNode) - Area next to the status indicator of the step.
	* * `header` (ReactNode) - Area next to the status indicator of the step.

feat: Steps #2684

feat: Steps #2684

Conversation

Revuj commented Sep 9, 2024

Description

How has this been tested?

Correctness

Security

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cansuaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Revuj Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Revuj Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Revuj Sep 9, 2024 •

edited

Loading

Revuj Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading