Skip to content
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

fix(dropdowns): pass user ID's to Downshift #6326

Merged

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Jun 22, 2020

Now without my goofy busted commit history ❤️

Closes #6304

In our updates we broke functionality for user supplied IDs getting passed down to Downshift. With this new implementation if the user provides a value for the id prop Downshift will create an id prop using that supplied id for label, menu etc appending the name of the element as intended (id="address" would result in id="address-label", id="address-menu" etc on the child components)

Changelog

  • Combobox: move id prop to wrapper component
  • Dropdown: move id prop to selectProps object
  • Multiselect: pass id prop to useSelect and remove unused label IDs

Testing / Reviewing

Go in the changed file and change the value for the id prop to something distinct. Then open dev tools on the Storybook for that element and search for that new value which should be seen on label, field, and toggle buttons for each component.

@dakahn dakahn requested a review from a team as a code owner June 22, 2020 23:21
@netlify
Copy link

netlify bot commented Jun 22, 2020

Deploy preview for carbon-elements ready!

Built with commit 768d8d4

https://deploy-preview-6326--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jun 22, 2020

Deploy preview for carbon-components-react ready!

Built with commit e933632

https://deploy-preview-6326--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Jun 23, 2020

Deploy preview for carbon-components-react ready!

Built with commit 768d8d4

https://deploy-preview-6326--carbon-components-react.netlify.app

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combobox looking great, a small change with Dropdown and Multiselect

packages/react/src/components/Dropdown/Dropdown.js Outdated Show resolved Hide resolved
packages/react/src/components/MultiSelect/MultiSelect.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Collaborator

I think snapshots just need to be updated so they don’t have [Object object]

@joshblack
Copy link
Contributor

Doing some quick digging (this is definitely grounds for some clean-up in the future)

The version we're trying to get to matches:

  • Dropdown places id on the bx--dropdown bx--listbox node
  • MultiSelect places id on the bx--multi-select bx--list-box
  • Combobox places the id on the input node

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the custom id supposed to be on the wrapper element or is that for a future PR?

image

@joshblack
Copy link
Contributor

@emyarod for dropdown it should show up on that node, if I understand correctly

@dakahn
Copy link
Contributor Author

dakahn commented Jun 24, 2020

@emyarod @joshblack yeah, I believe the node should be where it ends up yeah

@joshblack
Copy link
Contributor

Looking at the preview, seems like we get:

Dropdown id on bx--dropdown bx--listbox
Screen Shot 2020-06-24 at 10 51 07 AM

Combobox id on input
Screen Shot 2020-06-24 at 10 51 07 AM

MultiSelect id on bx--multi-select bx--listbox
Screen Shot 2020-06-24 at 10 51 13 AM

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, I couldn't replicate the issue in the screenshot earlier consistently but it might have just been a storybook problem since the snapshots look fixed now

@joshblack joshblack merged commit aa8060f into carbon-design-system:master Jun 24, 2020
gracelo added a commit to gracelo/carbon that referenced this pull request Jun 30, 2020
* origin/master: (77 commits)
  feat(pictograms): add systems devops category and pictograms (carbon-design-system#6332)
  fix(button): prevent tooltip from showing on incorrect hover (carbon-design-system#6349)
  feat(pictograms): add coronavirus pictogram in Life Science category (carbon-design-system#6351)
  chore(npmignore): update npmignore file (carbon-design-system#6350)
  chore(project): sync generated files
  fix(pagination): set correct width on ghost buttons (carbon-design-system#6344)
  docs(text-area): rename story file to use camel case in component name (carbon-design-system#6334)
  chore(release): v10.15.0-rc.0 (carbon-design-system#6343)
  feat(pictograms): New add device pictogram (carbon-design-system#6333)
  fix(Dropdown): add downshiftProps handling (carbon-design-system#6341)
  feat(pictograms): add supply chain category with new pictograms (carbon-design-system#6305)
  feat(pictograms): add gift pictogram in retail (carbon-design-system#6336)
  feat(pictograms): add blender pictogram in lifestyle category (carbon-design-system#6318)
  fix(dropdowns): pass user ID's to Downshift (carbon-design-system#6326)
  fix(carbon): revert pr 6185 (carbon-design-system#6331)
  feat(react): remove unsafe lifecycle methods from DataTable and DatePicker (carbon-design-system#6307)
  docs(handbook): add icon docs to developer-handbook (carbon-design-system#6300)
  feat(pictograms): add planning analytics pictogram to cloud category (carbon-design-system#6320)
  chore(project): sync generated files
  fix(loading): use `$overlay-01` token for overlay (carbon-design-system#6323)
  ...

# Conflicts:
#	packages/components/src/components/tabs/_tabs.scss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown variants should support user-provided id
4 participants