Add Visual Picker Component

[aswinshenoy]

Fixes #1693

Hi @interactivellama, @futuremint!

Added an initial draft for the Visual Picker Component from SLDS, feedback would be much appreciated.

Additional description

Currently has -

• VisualPicker Component

• Coverable Component (Will Rename / Merge)

• NonCoverable Component (Will Rename / Merge)

I am still working on adding support for Vertical Picker and Link Picker
Also, will look onto merging coverable, non-coverable and other children of visual picker into one type.

Screenshots

Coverable Visual Picker

Non-Coverable Visual Picker

---

CONTRIBUTOR checklist (do not remove)

Please complete for every pull request

• First-time contributors should sign the Contributor License Agreement. It's a fancy way of saying that you are giving away your contribution to this project. If you haven't before, wait a few minutes and a bot will comment on this pull request with instructions.
• 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/.
• The Accessibility panel of each Storybook story has 0 violations (aXe). 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.

REVIEWER checklist (do not remove)

• TravisCI tests pass. This includes linting, Mocha, Jest, Storyshots, and components/component-docs.json tests.
• Tests have been added for new props to prevent regressions in the future. See readme.
• Review the appropriate Storybook stories. Open http://localhost:9001/.
• The Accessibility panel of each Storybook story has 0 violations (aXe). 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.

Required only if there are markup / UX changes

• 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.

[interactivellama]

A five-minute look at the examples looks like this is the right direction and looking great! I may need a few more days to re-review my own props proposal and look this pull request more in-depth.

https://design-system-react-co-pr-1786.herokuapp.com/?selectedKind=SLDSVisualPicker&selectedStory=Coverable&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

[interactivellama]

There are some text size issues with the coverable variant.

From https://www.lightningdesignsystem.com/components/visual-picker/

[interactivellama]

Functionally, this is working well.

Since this is essentially a pretty radio and checkbox group. I'd like to mirror the API of Radio Button Group https://github.com/salesforce/design-system-react/blob/master/components/radio-button-group/__docs__/storybook-stories.jsx#L34 and Button Group (Checkbox variant)

I'm thinking something like:

<VisualPicker content="coverable" variant="radio" // this is actually "base" on the SLDS site
  <Radio label="Connected App" onRenderVisualPickerNotSelected={<Icon iconName="lightning" />} /> // clone radio child and pass in variant="visual-picker-coverable"
</VisualPicker>

This would allow custom icons and other things like custom SVG. What do you think?

[aswinshenoy]

So, is it going to be like https://github.com/salesforce/design-system-react/blob/master/components/button-group/index.jsx, where we will clone <Radio> / <Checkbox> and render?
Or are we going to create a new variant for and like button-group variant in <Radio >. Probably, will need to create variants, as we need to have props to enter Icons etc.
I will start looking through it tomorrow 👍

[interactivellama]

Yes, I think these would clone and add a variant="visual-picker" prop. The child render function be located in the radio/index.jsx file. I think putting it there will remind consumers that this is a decorated Radio button and they shouldn't be putting <button> and other interactive items in the onRender function.

My main thought on this is make sure consumers know it's a Radio or Checkbox, so that they know it has limitations.

[kevinparkerson]

The prop label has been deprecated for checkbox. This will need to be this.props.labels.label

[kevinparkerson]

In regards to the .slds-text-heading_small and .slds-text-title classed items:

• First, radio will need to have the label property deprecated. The current property is marked as isRequired. Don't worry about this and remove it anyway
• A labels object property will be added to radio, as seen in checkbox. Backwards compatibility will be needed to support the deprecated label property, which is done in checkbox here
• Both checkbox and radio will have a new attribute on the props.labels object called heading
• Both checkbox and radio will need a heading attribute added to the props.assistiveText object

Then, for visual-picker variants in both components:

• props.labels.heading will always be placed within .slds-text-heading_small. If not present the heading will not be rendered
• props.labels.label will always be placed within .slds-text-title
• assistive text will first attempt to use assistiveText.label and then fall back to assistiveText.heading

[interactivellama]

@aswinshenoy Unused variables found.
https://deepscan.io/dashboard/#view=project&tid=616&pid=5608&bid=43183&subview=pull-request&prid=111327

[kevinparkerson]

@aswinshenoy these onRender{Item} props need to be functions, and then in the examples the content should be returned from those functions:
https://github.com/salesforce/design-system-react/blob/master/docs/codebase-overview.md#prefix-render-props-with-onrender

[interactivellama]

onRender callbacks should be function instead of type node

Although, I guess that isn't very clear here and assumes that:
https://github.com/salesforce/design-system-react/blob/master/docs/codebase-overview.md#prefix-render-props-with-onrender

[interactivellama]

This should be of type function along with the other render props in radio

[interactivellama]

This version is wrong. The whole line can be removed though.

[interactivellama]

The icons in this vertical coverable example should have text in it. Currently it's undefined, so the screen reader doesn't read anything. The name of the icon would be fine since this example doesn't exist in SLDS.

connected_apps or check

The goes for the checkbox examples in this story.

If you run npm run test:accessibility (I would recommend commenting out all of the components in story-based-tests.jsx except VisualPicker in order to speed things up.), you will see:

aXe Testing
    aXe storyshots
      SLDSVisualPicker
        ✓ Coverable (2646ms)
        ✓ Non-Coverable (2902ms)
        ✓ Links (518ms)
        ✓ Vertical (2295ms)
        ✕ Vertical Coverable (1938ms)

  ● aXe Testing › aXe storyshots › SLDSVisualPicker › Vertical Coverable

    expect(received).toHaveNoViolations(expected)

    Expected the HTML found at $('#visual-picker-vertical-coverable-radio-1') to have no violations:

    <input type="radio" id="visual-picker-vertical-coverable-radio-1" name="visual-picker-vertical-coverable-radio_options" value="">

    Expected the HTML found at $('#visual-picker-vertical-coverable-radio-2') to have no violations:

    <input type="radio" id="visual-picker-vertical-coverable-radio-2" name="visual-picker-vertical-coverable-radio_options" value="">

    Expected the HTML found at $('#visual-picker-vertical-coverable-checkbox-1') to have no violations:

    <input id="visual-picker-vertical-coverable-checkbox-1" name="visual-picker-vertical-coverable-checkbox_options" type="checkbox">

    Expected the HTML found at $('#visual-picker-vertical-coverable-checkbox-2') to have no violations:

    <input id="visual-picker-vertical-coverable-checkbox-2" name="visual-picker-vertical-coverable-checkbox_options" type="checkbox">

    Expected the HTML found at $('#visual-picker-vertical-coverable-checkbox-3') to have no violations:

    <input id="visual-picker-vertical-coverable-checkbox-3" name="visual-picker-vertical-coverable-checkbox_options" type="checkbox">

    Received:

    "Form elements must have labels (label)"

    Try fixing it with this help: https://dequeuniversity.com/rules/axe/3.2/label?application=axeAPI

      65 | 			const component = story.render(context);
      66 | 			const wrapper = mount(component);
    > 67 | 			expect(await axe(wrapper.html())).toHaveNoViolations();
         | 			                                  ^
      68 | 		},
      69 | 	});
      70 | });

[interactivellama]

@aswinshenoy I've completed my review. Please update and I think we can merge.

[kevinparkerson]

@aswinshenoy please add VisualPicker to components/index.js

export SLDSVisualPicker from './visual-picker';
export VisualPicker from './visual-picker';

export SLDSVisualPickerLink from './visual-picker/link';
export VisualPickerLink from './visual-picker/link';

[kevinparkerson]

@aswinshenoy please add the contents of VisualPicker's docs.json to the project root package.json file under "components". It should be added in alphabetical order but in this case that means at the very end :)

[kevinparkerson]

This seems good to go now!

[Rancho06]

Hi @kevinparkerson @aswinshenoy wondering any estimates when the visual picker will be released? Will this be part of next 0.10.6 release?

[interactivellama]

@Rancho06 Most likely next week. You can clone and run npm run dist and use /.tmp-npm in your node_modules folder locally until it is released.

[Rancho06]

thanks @interactivellama That's very good to know!