Skip to content

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

Merged

Conversation

kchan902
Copy link
Contributor

@kchan902 kchan902 commented May 13, 2018

… completed steps. Also added in assistive text messages for completed and disabled steps

Fixes #1238 and #1374

Additional description

  • Adds assistiveText props
    • activeStep: Label for the active step. The default is Active Step
    • completedStep: Label for a completed step. The default is Completed Step
    • disabledStep: Label for disabled step. The default is Disabled Step
    • errorStep: Label for a step with an error. The default is Error Step
    • step: Label for a step. It will be typically followed by the number of the step such as "Step 1".
  • Update error icon
  • add aria-current=step

Pull Request Review checklist (do not remove)

  • npm run lint:fix has been run and linting passes.
  • Mocha, Jest (Storyshots), and components/component-docs.json CI tests pass (npm test).
  • Tests have been added for new props to prevent regressions in the future. See readme.
  • Review the appropriate Storybook stories. Open http://localhost:9001/.
  • Review tests are passing in the browser. Open http://localhost:8001/.
  • Review markup conforms to SLDS by looking at DOM snapshot strings.
  • Add year-first date and commit SHA to last-slds-markup-review in package.json and push.
  • Request a review of the deployed Heroku app by the Salesforce UX Accessibility Team.
  • Add year-first review date, and commit SHA, last-accessibility-review, to package.json and push.
  • While the contributor's branch is checked out, run npm run local-update within locally cloned site repo to confirm the site will function correctly at the next release.

… completed steps. Also added in assistive text messages for completed and disabled steps
@kchan902
Copy link
Contributor Author

@interactivellama Could you please review this pull request when you have a chance? I am not able to assign it for review. Thank you!

@interactivellama interactivellama self-requested a review May 14, 2018 20:52
{props.step.assistiveText || `Step ${props.index + 1}: ${status}`}
{(() => {
if (props.isCompleted) {
return "Completed: " +
Copy link
Contributor

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 || "");
Copy link
Contributor

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
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
@salesforce-cla
Copy link

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
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 5, 2018 20:39 Inactive
@interactivellama
Copy link
Contributor

interactivellama commented Jul 5, 2018

@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 5, 2018 20:48 Inactive
@interactivellama
Copy link
Contributor

Summarizing @arodenbeck

  • Remove "Active Step"
  • Separate disabled and error text from label name with "-". Remove "step" from "Disabled Step" or "Error Step"
  • Make label format: ""Step 2: Email Attributes - Disabled"
  • disabled step should be a with role=button
  • Remove tabindex="0" from progress bar node.

@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 18, 2018 04:50 Inactive
completedStep: 'Completed Step',
disabledStep: 'Disabled Step',
errorStep: 'Error Step',
activeStep: '',
Copy link
Contributor

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.

@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 19, 2018 21:57 Inactive
@arodenbeck
Copy link

arodenbeck commented Jul 20, 2018

@kchan902 Looking good. I did notice a few cosmetic things which might not exist in production, so let me know if these are irrelevant.

  • The step names seem to have the - separator as part of their label. This causes a problem for steps which do not have a Disabled or Error tag at the end. Example "undefined 4: tooltip label Use PropTypes for PicklistBase Function #4 -" There is a trailing -
  • The variant "Completed progress", has "Finished step" rather than "Completed." It looked like the messaging can be configured by the person using the component, so was this just defined as an example?
  • Several of the variants use a label of "This is custom text in the assistive text key" for the first step. I know it is up to the consuming dev to define the names of the steps, but that's a strange message.
  • This might be big enough to stand as its own issue, and I'll be glad to create one separate if you think it necessary. Each step triggers a tooltip on focus with a of the label of the step. I think that was part of the initial design, but that functionality is unnecessary, and doesn't match the current SLDS version. This tooltip style functionality should be removed. (step label is repeated twice and should on be once, should assistive text span be removed)

@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 25, 2018 05:18 Inactive
@kchan902
Copy link
Contributor Author

@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
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 25, 2018 23:19 Inactive
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 25, 2018 23:38 Inactive
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 25, 2018 23:46 Inactive
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 25, 2018 23:55 Inactive
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 26, 2018 00:00 Inactive
@interactivellama interactivellama temporarily deployed to design-system-react-co-pr-1376 July 26, 2018 03:16 Inactive
- Remove trailing - on step without status
- Pass correct assistiveText to step
- Remove activeStep from docs
@interactivellama interactivellama merged commit 473c75b into salesforce:master Jul 26, 2018
interactivellama added a commit to interactivellama/design-system-react that referenced this pull request Aug 27, 2018
…icator-assistive-text"

This reverts commit 473c75b, reversing
changes made to 8fd3f2b.

# Conflicts:
#	tests/__snapshots__/story-based-tests.snapshot-test.js.snap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progress Indicators: Needs updates to the assistive-text
4 participants