Object Switcher: Added props for combobox #1638

ghost · 2018-11-02T16:05:44Z

support cross-entity
loading
default value
disabled
Menu: Footer and Header

Fixes #1600

Additional description

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.

- support cross-entity - loading - default value - disabled - Menu: Footer and Header

ghost · 2018-12-19T12:35:21Z

Hello Stephen, are you going to merge this PR or something wrong with it ?
I saw that you deployed it month ago.

interactivellama · 2018-12-19T15:54:32Z

@seirykborys I apologize if I was unclear.

The deployment was of your pull request branch's code and not a release of the library. You can see your code deployed to a Heroku (PaaS) "reivew app" here.
https://design-system-react-co-pr-1638.herokuapp.com/?selectedKind=SLDSCombobox&selectedStory=Inline%20Single%20Search%2FAdd%20Entities&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

My request was to add the header (Search salesforce) and the footer (Add item) of the menu items to the keyboard (arrow keys up/down) navigation to make the new menu items you added accessible to keyboard users. I was waiting for this feature before I continued to review and merge the code in.

The other option is that I believe I have time in the next few weeks to complete this ask if you'd prefer me to finish up the feature.

tanhengyeow · 2019-03-05T11:13:59Z

@seirykborys @interactivellama Would be more than happy to move this forward if y'all are occupied 😄

interactivellama · 2019-03-13T05:52:50Z

@tanhengyeow Yes, that would be great. It's mostly done. I haven't looked at it in a while, but the primary issue is that the menu list header and footer are not in the index when the keyboard arrows are used. They should be accessible for keyboard users just like the other items.

interactivellama · 2019-04-22T16:37:34Z

@davidlygagnon Would you be willing to finish this PR up. This is mostly an accessibility ask to complete. You know Combobox so well, I think knocking this out wouldn't be too difficult.

davidlygagnon · 2019-04-30T05:00:30Z

Sounds good - I'll wrap this up.

davidlygagnon · 2019-05-19T06:15:22Z

components/combobox/combobox.jsx

+	/**
+	 * Provided to input to make it disabled
+	 */
+	disabled: PropTypes.bool,


Not sure why he added this? We already have singleInputDisabled. We would need to figure out what it means to disable the input for the cases where it's not a single select.

Please remove. There will never be an all variants disabled prop on a combobox. If folks want to hack with with input prop. They can I think.

davidlygagnon · 2019-05-19T06:16:18Z

components/combobox/combobox.jsx

+	/**
+	 * Default value of input. Provide uncontroled behaviour
+	 */
+	defaultValue: PropTypes.string,


Should we call this defaultInputValue ?

Can this be added in the input prop? If so, please remove. I'd like to not promote it.

davidlygagnon · 2019-05-19T06:22:30Z

components/combobox/combobox.jsx

+	/**
+	 * Object for creating Add item below the options
+	 */
+	optionsAddItem: PropTypes.shape({


@interactivellama Wouldn't it be better to include this as part of the options object and have a different type attribute ? Something like option.type='header' and option.type='footer' Same question goes for optionsSearchEntity ? Under the hood, it was much cleaner to "merge" those 2 props with options to get the same keyboard nav behavior and highlighting on mouse over. Plus, we wouldn't need to have 2 extra props.

Other question is: If we keep as is, I'd suggest having a more generic name: optionsHeaderItem and optionsFooterItems ?

Also, should it accept an array instead of just one prop?

I think the idea was that you wouldn't want to pass it through the filter like the other items on keypress--and that the data type is different since it's not data, it's UI chrome in a way.

As to type, I think I've seen multi-entity searching on Platform such as Accounts, Users, etc. so an array might make more sense. My original proposal called for arrays. I don't think I had caught it in the previous review. Good catch!

As to naming, I was hoping to keep it semantic just in case it ever changed places. such as Add Item moved to the top or something. Do you foresee different footer needs. As you say, it might be good to future proof it as a head/footer. I could go either way.

interactivellama · 2019-08-20T21:48:18Z

Replacing with #1986

salesforce-cla bot added the cla:signed label Nov 2, 2018

added for combobox

80c1bcd

- support cross-entity - loading - default value - disabled - Menu: Footer and Header

interactivellama temporarily deployed to design-system-react-co-pr-1638 November 2, 2018 19:12 Inactive

interactivellama added the combobox label Nov 2, 2018

Merge branch 'master' into combobox_props

b66582b

interactivellama temporarily deployed to design-system-react-co-pr-1638 November 16, 2018 09:53 Inactive

Merge branch 'master' into combobox_props

544524c

interactivellama temporarily deployed to design-system-react-co-pr-1638 December 19, 2018 15:34 Inactive

interactivellama changed the title ~~added props for combobox~~ Object Switcher: Added props for combobox Jan 14, 2019

interactivellama added current sprint and removed current sprint labels Feb 11, 2019

interactivellama deployed to design-system-react-co-pr-1638 March 5, 2019 20:44 View deployment

interactivellama assigned kevinparkerson Apr 22, 2019

interactivellama assigned davidlygagnon and unassigned kevinparkerson Apr 22, 2019

pulkonet mentioned this pull request May 2, 2019

Components Checklist: Exhaustive checklist of components to be added to align DSR with SLDS #1933

Closed

davidlygagnon mentioned this pull request May 19, 2019

Adding header and footer items for combobox. #1986

Merged

17 tasks

davidlygagnon reviewed May 19, 2019

View reviewed changes

stale bot added the stale label Jul 18, 2019

salesforce deleted a comment from stale bot Jul 18, 2019

stale bot removed the stale label Jul 18, 2019

interactivellama added this to the 2019-10 Release cycle milestone Aug 6, 2019

interactivellama self-requested a review August 20, 2019 06:03

interactivellama closed this Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object Switcher: Added props for combobox #1638

Object Switcher: Added props for combobox #1638

ghost commented Nov 2, 2018 •

edited by ghost

Loading

ghost commented Dec 19, 2018

interactivellama commented Dec 19, 2018

tanhengyeow commented Mar 5, 2019

interactivellama commented Mar 13, 2019

interactivellama commented Apr 22, 2019

davidlygagnon commented Apr 30, 2019

davidlygagnon May 19, 2019

interactivellama May 19, 2019

davidlygagnon May 19, 2019

interactivellama May 19, 2019 •

edited

Loading

davidlygagnon May 19, 2019

interactivellama May 19, 2019

interactivellama commented Aug 20, 2019

Object Switcher: Added props for combobox #1638

Object Switcher: Added props for combobox #1638

Conversation

ghost commented Nov 2, 2018 • edited by ghost Loading

Additional description

CONTRIBUTOR checklist (do not remove)

REVIEWER checklist (do not remove)

Required only if there are markup / UX changes

ghost commented Dec 19, 2018

interactivellama commented Dec 19, 2018

tanhengyeow commented Mar 5, 2019

interactivellama commented Mar 13, 2019

interactivellama commented Apr 22, 2019

davidlygagnon commented Apr 30, 2019

davidlygagnon May 19, 2019

Choose a reason for hiding this comment

interactivellama May 19, 2019

Choose a reason for hiding this comment

davidlygagnon May 19, 2019

Choose a reason for hiding this comment

interactivellama May 19, 2019 • edited Loading

Choose a reason for hiding this comment

davidlygagnon May 19, 2019

Choose a reason for hiding this comment

interactivellama May 19, 2019

Choose a reason for hiding this comment

interactivellama commented Aug 20, 2019

ghost commented Nov 2, 2018 •

edited by ghost

Loading

interactivellama May 19, 2019 •

edited

Loading