feat: Add onClick optional property to ProgressStepper #2973
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
leeeandroo
changed the title
leeeandroo
force-pushed
the
feat/optional-onclick-progress-stepper
branch
from
2d11469 to
c35f31c
Compare
There was a problem hiding this comment.
Overall this approach looks great! ⭐ I added a comment about the impact of changing the step from a span to a button on the step styles (numbers and icons are no longer centered). I also think it would be great to add another example to the component documentation showing how to use this new onClick prop.
| const isClickable = onClick && state !== 'disabled'; | ||
|
|
||
| return isClickable ? ( | ||
| <button |
There was a problem hiding this comment.
When this is changed to a button element, it is now affecting the spacing of the numbers/icons within the step circle. When the stepStyle is number, the change is ever so slight, but the number is no longer centered:
| Not clickable (span element) | Clickable (button element) |
|---|---|
 |
 |
Also, when the component uses the icon stepStyle, the impact is more noticable:
| Not clickable (span element) | Clickable (button element) |
|---|---|
 |
 |
There was a problem hiding this comment.
Thanks for checking @whitelisab !
I could not simulate the misalignment on that level you show in the screenshots. But I did some changes, could you please check it again? (And let me know how you saw it in this way, so next time I can check it properly)
There was a problem hiding this comment.
Introducing the button requires updating the focus style when navigating with the keyboard.
focus-tab.mov
As you can see in the video, we don't see the step number focused state on steps with a dark blue border. All focusable elements across the Design System components use the tokens.glowPrimary style for their focus ring.
With that said, here are my suggestions:
- for the step number, use the
tokens.glowPrimaryinstead of the current dark blue color - for the label that becomes a button, use the Forma 36 Button component instead. You will inherit the right focus glow, it also ensures consistency with the use of actionable components across the app. It could be used with the
smallsize andtransparentvariant, with additional custom styling to reduce its width/height, if necessary.
@damann do we also need to update the Figma component to introduce the onClick feature?
There was a problem hiding this comment.
Thank you for checking and the well explained suggestions @cf-remylenoir !
for the step number, use the tokens.glowPrimary instead of the current dark blue color
Done!
for the label that becomes a button, use the Forma 36 Button component instead. You will inherit the right focus glow, it also ensures consistency with the use of actionable components across the app. It could be used with the small size and transparent variant, with additional custom styling to reduce its width/height, if necessary.
I have changed the button to the f36 Button component, but I have also added the tabIndex as -1, so user will only navigate to the step number/icon. Let me know if you think it is OK
leeeandroo
force-pushed
the
feat/optional-onclick-progress-stepper
branch
from
9f26d59 to
264b084
Compare
|
@leeeandroo I tried the chromatic generated storybook, I think the labels below the step number should also be clickable. What do you think? |
That's a good point @john-e , thanks for the suggestion. Added, please have a look again 🙏 |
feat: Add onClick optional property to
ProgressStepperPurpose of PR
PR Checklist
readme.mdfile(s)