Updating OutsideClickHandler to accept "flex" as a display prop #4
Conversation
src/OutsideClickHandler.jsx
Outdated
| @@ -112,7 +113,7 @@ export default class OutsideClickHandler extends React.Component { | |||
| return ( | |||
| <div | |||
| ref={this.setChildNodeRef} | |||
| style={display === DISPLAY.INLINE_BLOCK ? { display } : undefined} | |||
| style={Object.values(DISPLAY).includes(display) ? { display } : undefined} | |||
There was a problem hiding this comment.
this is needlessly O(2n), this should be has(DISPLAY, display) instead; and part of the goal is that when it's "block", there's no style, since that's the default for divs
There was a problem hiding this comment.
@ljharb Fair enough, didn't want to bring in lodash unnecessarily but if it's alright with you then sounds good to me.
There was a problem hiding this comment.
Absolutely not lodash; I meant the "has" package.
There was a problem hiding this comment.
not sure how has would work here as it checks with hasOwnProperty, not for values, and the display prop being passed in would either be block, inline-block, or flex which are values not keys of the lookup table DISPLAY
There was a problem hiding this comment.
oh true, that's a good point :-)
ok in that case, i guess this is fine (we'd need to use https://npmjs.com/object.values tho, and not the builtin) - altho it's probably better to build up a reverse object at module level, and then use has here.
src/OutsideClickHandler.jsx
Outdated
| @@ -112,7 +113,7 @@ export default class OutsideClickHandler extends React.Component { | |||
| return ( | |||
| <div | |||
| ref={this.setChildNodeRef} | |||
| style={display === DISPLAY.INLINE_BLOCK ? { display } : undefined} | |||
| style={Object.values(DISPLAY).includes(display) ? { display } : undefined} | |||
There was a problem hiding this comment.
Since div is BLOCK by default, part of the original code path was specifically to not apply a style in the block case. Can we bring back that functionality here?
|
Also added in an update to the README to match the addition of flex as an option |
README.md
Outdated
| @@ -44,6 +44,6 @@ See https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Ev | |||
|
|
|||
| If `useCapture` is true, the event will be registered in the capturing phase and thus, propagated top-down instead of bottom-up as is the default. | |||
|
|
|||
| ### display: `PropTypes.oneOf(['block', 'inline-block'])` | |||
| ### display: `PropTypes.oneOf(['block', 'flex, 'inline-block'])` | |||
There was a problem hiding this comment.
Not a major deal since it's the README, but you're missing a closing quote on flex.
|
@erikrahm yes, please revert any unrelated formatting changes. |
|
@ljharb Fixed |
|
Would you mind rebasing this down to a single commit? |
There are tons of different ways to handle the ternary for the style property on the main `div`, so feel free to chime in with something else if you think it would be more efficient. Performing requested updates to patch-1 Updating the README.md to match the addition of flex README formatting issue Adding a missing quite to README Reverting formatting changes Reverting formatting changes brought on by text editor using prettier. Bold to italics Reverting bold to italics in the README
|
@ljharb squashed 😁 |
|
Yeah @ljharb , added in the |
|
@ljharb Will this be merged soon? |
|
@brandonb81 it's been less than 24 hours since it was finalized, so I'm sure if you have some patience, it will be merged when it's ready. |
|
🖖🏻 |
There are tons of different ways to handle the ternary for the style property on the main
div, so feel free to chime in with something else if you think it would be more efficient.