-
Notifications
You must be signed in to change notification settings - Fork 433
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
assistiveTextpropsactiveStep: Label for the active step. The default isActive StepcompletedStep: Label for a completed step. The default isCompleted StepdisabledStep: Label for disabled step. The default isDisabled SteperrorStep: Label for a step with an error. The default isError Stepstep: Label for a step. It will be typically followed by the number of the step such as "Step 1".aria-current=stepPull Request Review checklist (do not remove)
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.