-
Notifications
You must be signed in to change notification settings - Fork 432
Add Select component #1791
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
Add Select component #1791
Conversation
|
Initial thoughts: |
|
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
interactivellama
left a comment
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.
I left some comments on the code. DOM snapshot tests still need to be added.
| import React from 'react'; | ||
| import IconSettings from '~/components/icon-settings'; | ||
| import Select from '~/components/select'; | ||
| import SelectContainer from '../private/container'; |
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.
No components in private folder should be used by consumers or in examples
| import IconSettings from '~/components/icon-settings'; | ||
| import Select from '~/components/select'; | ||
| import SelectContainer from '../private/container'; | ||
| import Label from '../private/label'; |
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.
Label should be a prop of Select component.
| render() { | ||
| return ( | ||
| <IconSettings iconPath="/assets/icons"> | ||
| <Select> |
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.
The outputted markup is good, but this should all be one component called Select, since I don't foresee any rearrangement of these such as the label after the select element, etc.
|
Based on LinkedIn message with @tanhengyeow I'm going to close this pull request. Feel free to pick it back up when you have time to complete it. Thanks! |
…to pr/tanhengyeow/1791 # Conflicts: # components/index.js
Fixes #1691
Hi @interactivellama ! Feedback for the initial draft would be appreciated :)
Select:
SelectContainer:
Error:
Some screenshots for reference below.
Base:

Required:

Error:

Disabled:

Multiple Selection Narrow:

I'll add support for
Multiple Selectiononce #1712 is fixed.Additional description
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).REVIEWER checklist (do not remove)
components/component-docs.jsontests.Required only if there are markup / UX changes
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.