-
Notifications
You must be signed in to change notification settings - Fork 204
feat: add custom primary actions for wizard component #3994
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
Conversation
just-boris
left a comment
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.
There is a failing build. Let's fix this first and then I will review the rest
914124c to
08633b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3994 +/- ##
=======================================
Coverage 97.18% 97.18%
=======================================
Files 858 858
Lines 25345 25348 +3
Branches 9004 9007 +3
=======================================
+ Hits 24632 24635 +3
Misses 664 664
Partials 49 49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }; | ||
|
|
||
| const customPrimaryActions = ( | ||
| <SpaceBetween size="m" direction="horizontal"> |
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.
We should be using xs spacing size between buttons. There is a mention of this in our docs: https://cloudscape.design/components/button/?tabId=api
| id="wizard" | ||
| steps={steps} | ||
| i18nStrings={i18nStrings} | ||
| allowSkipTo={true} |
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 does this prop do in this case? if we render a fully custom actions, this property has no effect, right?
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.
Correct. The property won't have an effect in this case. It's in the dev demo pages and I used it for manual testing.
src/wizard/__tests__/wizard.test.tsx
Outdated
| DEFAULT_STEPS.forEach((_, index) => { | ||
| const customActionButtonWrapper = wrapper.findActions()!.findButton('[data-testid="custom-action"]'); | ||
| expect(customActionButtonWrapper).not.toBeNull(); | ||
| customActionButtonWrapper?.click(); |
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 should be a strict non-null assertion, !. Because otherwise the test silently skips this line
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.
The customActionButtonWrapper is asserted the line before, so it would throw an error before. But I'll fix it to be clearer.
src/wizard/__tests__/wizard.test.tsx
Outdated
| expect(wrapper.findActions()!.findButton('[data-testid="custom-next"]')).not.toBeNull(); | ||
| }); | ||
|
|
||
| test('custom primary actions work on all steps', () => { |
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.
Is it really needed to check on all steps? I think it would be better to check 3 corner-cases specifically 1) first step 2) any middle step 3) last step
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.
The DEFAULT_STEPS contains 3 steps, so it'll check the first, middle and last step or would you suggest separating the test into three separate ones?
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.
yes, 3 separate ones would be easier to read and understand why they exist
src/wizard/interfaces.ts
Outdated
| allowSkipTo?: boolean; | ||
|
|
||
| /** | ||
| * Specifies right-aligned custom primary actions for the wizard. Overwrites existing buttons (e.g. Cancel, Next, Finish, ...). |
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 ...). punctuation in the end looks strange, you will need to check with a designer if there is a better way
- Change SpaceBetween size from 'm' to 'xs' in custom primary actions - Remove unnecessary allowSkipTo prop - Replace original chaining with non-null assertion in tests - Remove debug.console.log statement
Description
Adds functionality to overwrite the content of the primary actions for wizards (e.g. Next, Previous, Cancel, ...).
Currently, the Wizard only allows the default primary actions (Next, Previous, Cancel, Finish, Skip to ...). To allow further flexibility, a
customPrimaryActions-property was introduced to allow custom actions.Related links, issue #, if available: n/a
How has this been tested?
Unit Testing
Manual Testing
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.