Create OrderSelect and Label components #905
Conversation
Aljullu
added
status: needs review
type: refactor
| } | ||
|
|
||
| if ( ! label && screenReaderLabel ) { | ||
| if ( typeof Wrapper === 'symbol' && Symbol.keyFor( Wrapper ) === 'react.fragment' ) { |
There was a problem hiding this comment.
I'm not sure doing this typeof check for 'symbol' will work in IE11 even with polyfill (should be verified). Instead you could probably do something like:
if ( label.type && label.type === Fragment )There was a problem hiding this comment.
Checking the type didn't seem to work, but I refactored the component so we don't need to check for Symbol anymore: d398806. Tested and it works fine on IE11. 👍
| import Label from '../label'; | ||
| import './style.scss'; | ||
|
|
||
| const OrderSelect = ( { className, componentId, defaultValue, label, onChange, options, screenReaderLabel, readOnly, value } ) => { |
There was a problem hiding this comment.
What is OrderSelect? I think it would be helpful to include some doc block here explaining what the OrderSelect component is used for.
package.json
Outdated
| @@ -86,6 +86,7 @@ | |||
| "postcss-loader": "3.0.0", | |||
| "progress-bar-webpack-plugin": "1.12.1", | |||
| "promptly": "3.0.3", | |||
| "prop-types-elementtype": "^1.0.0", | |||
There was a problem hiding this comment.
Doesn't look like this is needed? facebook/prop-types#211 - from the text on the `prop-types-elementtype repo README.md:
You can use this library until this PR is merged in the official PropTypes.
It looks like the prop-types library has PropTypes.element and PropTypes.elementType as options.
There was a problem hiding this comment.
Weird, I must have had an old version of prop-types installed locally because it wasn't working for me before.
Should be fixed now: 850dc62.
|
Thanks for the review @nerrad. This is ready for another look. |
| @@ -48,13 +54,12 @@ Label.propTypes = { | |||
| screenReaderLabel: PropTypes.string, | |||
| wrapperElement: PropTypes.oneOfType( [ | |||
| PropTypes.string, | |||
| elementType, | |||
| PropTypes.elementType, | |||
There was a problem hiding this comment.
Looking into this, it looks like under the hood, PropTypes.elementType is using react-is. So this type also validates string elements (like 'div', 'span' etc). So it should be safe to just remove the PropTypes.string here and only validate PropTypes.elementType.
There was a problem hiding this comment.
Good catch! Changed in 1fcd28a.
This PR creates an
<OrderSelect />component with most of the logic from<ReviewOrderSelect />so we have a generic component that can be used in All Products block.I also extracted some logic to display/hide label and screen reader labels into a
<Label />component, this way it can be shared between<OrderSelect />and<LoadMoreButton />.Screenshots
(this PR shouldn't produce any visible change)
How to test the changes in this Pull Request:
assets/js/base/components/label/test/__snapshots__/index.js.snapmarkup and verify it makes sense.Changelog