-
Notifications
You must be signed in to change notification settings - Fork 435
Fixed problem with progress indicator/assistive-text #1376
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
Fixed problem with progress indicator/assistive-text #1376
Conversation
… completed steps. Also added in assistive text messages for completed and disabled steps
@interactivellama Could you please review this pull request when you have a chance? I am not able to assign it for review. Thank you! |
…to progress-indicator-assistive-text
{props.step.assistiveText || `Step ${props.index + 1}: ${status}`} | ||
{(() => { | ||
if (props.isCompleted) { | ||
return "Completed: " + |
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.
Between assistiveText
and labels
, all text should be able to be replaced to allow internationalization. In the prior code, the following is on the step object, to allow full control.
{props.step.assistiveText || `Step ${props.index + 1}: ${status}`}
"Completed: "
can't be changed in this example.
{props.step.assistiveText || `Step ${props.index + 1}: ${status}`} | ||
{(() => { | ||
|
||
return "Disabled: " + (this.props.assistiveText.disabledStep || ""); |
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.
Between assistiveText
and labels
, all text should be able to be replaced to allow internationalization. In the prior code, the following is on the step object, to allow full control.
{props.step.assistiveText || `Step ${props.index + 1}: ${status}`}
"Disabled: "
can't be changed in this example.
…02/design-system-react into progress-indicator-assistive-text # Conflicts: # package-lock.json
…to progress-indicator-assistive-text
instead of hard coded text strings This also makes all tooltips info tooltips and removes use of error tooltips.
assistiveText: { activeStep: 'Active Step', completedStep: 'Completed Step', disabledStep: 'Disabled Step', errorStep: 'Error Step', step: 'Step' },
This maintains state and shows how to manipulate the component for devs
Thanks for the contribution! Before we can merge this, we need @kchan902 to sign the Salesforce.com Contributor License Agreement. |
This maintains state and shows how to manipulate the component for devs
…02/design-system-react into progress-indicator-assistive-text # Conflicts: # components/progress-indicator/__examples__/default.jsx
…02/design-system-react into progress-indicator-assistive-text # Conflicts: # components/progress-indicator/__examples__/default.jsx
…02/design-system-react into progress-indicator-assistive-text
@arodenbeck Please review https://design-system-react-co-pr-1376.herokuapp.com/?selectedKind=SLDSProgressIndicator&selectedStory=Step%20Error&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel and confirm it fixes the issue you created in January #1238. |
for error state
This allows the tooltip to be at the top
Summarizing @arodenbeck
|
completedStep: 'Completed Step', | ||
disabledStep: 'Disabled Step', | ||
errorStep: 'Error Step', | ||
activeStep: '', |
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.
Please remove completely from component codebase.
@kchan902 Looking good. I did notice a few cosmetic things which might not exist in production, so let me know if these are irrelevant.
|
@arodenbeck for points 2 and 3, yes it was defined as an example. for point 4, please create a separate issue. thanks! |
…to progress-indicator-assistive-text # Conflicts: # package-lock.json # tests/__snapshots__/story-based-tests.snapshot-test.js.snap
- Remove trailing - on step without status - Pass correct assistiveText to step - Remove activeStep from docs
4c5e80d
to
8d63d17
Compare
… completed steps. Also added in assistive text messages for completed and disabled steps
Fixes #1238 and #1374
Additional description
assistiveText
propsactiveStep
: Label for the active step. The default isActive Step
completedStep
: Label for a completed step. The default isCompleted Step
disabledStep
: Label for disabled step. The default isDisabled Step
errorStep
: Label for a step with an error. The default isError Step
step
: Label for a step. It will be typically followed by the number of the step such as "Step 1".aria-current=step
Pull Request Review checklist (do not remove)
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.