-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: parametrized StepIndicator strings #2707
Conversation
@@ -16,6 +16,8 @@ type StepIndicatorProps = { | |||
HTMLHeadingElement | |||
> | |||
headingLevel: HeadingLevel | |||
stepText?: string |
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.
If there are more of these, would it be better to do an object with multiple entries?
text: { step?: string, of?: string }
That way we can extend without adding loads more props?
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.
These are the only two strings on the top-level component, so would it still be beneficial to put it in such an object?
The other strings are on the StepIndicatorStep sub-component, which could use such a treatment, but I don't think we have that pattern on other components (yet) and wanted to keep this PR tight enough that we can unblock the project requirement.
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.
Okay no worries, that works. Lemme test.
<StepIndicator | ||
headingLevel="h2" | ||
ofText={args.ofText} | ||
stepText={args.stepText}> | ||
<StepIndicatorStep label="Personal information" status="complete" /> |
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.
Recommend removing the passed in text in one of the tests so that we can ensure the fallback to english works correctly.
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 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.
LGTM, tested in storybook.
One suggestion about ensuring the fallback is tested.
…swds into an-steptranslate-2705
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.
Tested and passed.
Summary
Parametrized strings in StepIndicator component to allow translation of "Step" and "of" strings.
There are still more English strings hard coded in the StepIndicatorStep subcomponent, as well as other components, but this narrow improvement is to allow for a client project to proceed. I will continue pushing for resources to prioritize an i18n strategy (see #1742)
Related Issues or PRs
Resolves #2705
How To Test
Screenshots (optional)