-
Notifications
You must be signed in to change notification settings - Fork 427
Convert Dropdown to ES6 class #1556
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
Convert Dropdown to ES6 class #1556
Conversation
|
||
MenuDropdown.contextTypes = { | ||
iconPath: PropTypes.string, | ||
}; | ||
|
||
MenuDropdown.propTypes = propTypes; | ||
MenuDropdown.defaultProps = defaultProps; |
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.
Why do we define these above the class then reference those variables here, rather than define them inline here only once? Is that just a DSR standard?
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.
Yeah, artifact of history--but also from my point of view, even though we have a website, there are still engineers who don't trust docs and look at the source code for prop types, so putting them first helps their dev experience.
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.
From what I can tell this seems just fine. My only comment is more just a question in regards to formatting vs anything that needs correction.
Prepare the way for #844 and #1089
Additional description
This removes the last mixin from non-deprecated components. (
menu-picklist
still has one, but Combobox should be used now instead of that).In order to do this I just copied, parts of the mix-in into menu-dropdown (thus created duplicate code. This PR also breaks apart the menu navigating mixin so part of it can be used if so desired by #1089.
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.