Components: Extract a generic dropdown component #2888
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2888 +/- ##
==========================================
+ Coverage 33.92% 34.19% +0.26%
==========================================
Files 190 191 +1
Lines 5695 5656 -39
Branches 999 992 -7
==========================================
+ Hits 1932 1934 +2
+ Misses 3183 3150 -33
+ Partials 580 572 -8
Continue to review full report at Codecov.
|
youknowriad
force-pushed
the
update/dropdown-menus
branch
from
f6b2b28
to
9f8e4ee
Compare
| } | ||
|
|
||
| clickOutside( event ) { | ||
| if ( ! this.container.contains( event.target ) ) { |
There was a problem hiding this comment.
Should we guard against falsy this.container?
There was a problem hiding this comment.
contains is inclusive :)
| return ( | ||
| <div className="blocks-color-palette"> | ||
| { colors.map( ( color ) => { | ||
| const style = { color: color }; |
There was a problem hiding this comment.
Oh forgot to update this before the merge.
|
|
||
| return ( | ||
| <div ref={ this.bindNode } className="editor-inserter"> | ||
| function Inserter( { position, children, onInsertBlock, insertionPoint } ) { |
|
Oh, for the record, one thing behaves slightly differently: toggling the Table of Contents popover. In
So, in a way, things are incidentally less broken in this branch. 😅 |
|
@mcsf I suspect this issue to be still present in this branch somehow, it has to do with how the Popover switches its position if there's not enough place. This is something we're aware of (I think there are issues for this), and we'd have to fix globally for all popovers. |
I suspected this, thanks for the context. |
|
@mcsf here's a talk explaining why renderProps may be better than HoCs. https://www.youtube.com/watch?v=BcVAq3YFiuc Unlike the speaker, I'm not all for one or another, it depends on the use-case. Composing HoC is nice sometimes. |
|
Oh, functions-as-props definitely have a lot of merit, but as with any abstraction it was good to compare the two for a specific case. |
| </Popover> | ||
| </div> | ||
| ) } | ||
| renderContent={ () => ( |
There was a problem hiding this comment.
Did you consider just passing this as children? Even if it were a function callback?
There was a problem hiding this comment.
Yes, I did, it was my first option, but then I hesitated whether the renderToggle should be the children function because it's the one being displayed as a child and not in a portal.
The answer was not obvious to me, which one to choose so I preferred avoiding it completely and being consistent between renderToggle and renderContent
When working on several UI elements, I found myself repeating the same pattern over and over again when needing a Dropdown component:
This PR factorize these into a single Dropdown component leveraging renderProps to allow any custom UI while factorizing the state updates and the events handlers (see the README for usage)
We still need the direct usage of
Popoverin one or two advanced use-cases.Checklist:
Testing instructions
Ensure that:
still work as expected