Fix bug introduced by #2443 where drop-down menu can't display #2492
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2492 +/- ##
==========================================
+ Coverage 31.28% 31.59% +0.31%
==========================================
Files 175 175
Lines 5283 5307 +24
Branches 909 916 +7
==========================================
+ Hits 1653 1677 +24
Misses 3080 3080
Partials 550 550
Continue to review full report at Codecov.
|
|
Thank you for working on this. The issue doesn't seem completely fixed for me, as the dropdown is now fixed in place when scrolling, and you can't actually scroll it into view:
However, I'm the cause of this issue! My apologies. So, the sliding scrollbar was my idea for an interim solution to improving the quick toolbar on mobile. Perhaps it deserves a little rethink? Right now we have an ellipsis menu just for the mover and cog. This isn't ideal. What if we ditched that ellipsis menu and just put the movers and cog into the quick toolbar itself, and let it scroll horizontally? This still wouldn't solve the dropdown problem on mobile. But what if we let the dropdown instead be a "prompt" style dropdown? Here's a mockup:
CC: @mtias @karmatosed |
Yeah, I knew that would likely be an issue as I'm currently not taking into account scrollLeft at all. In theory I just need to go up the hierarchy until I hit the offsetParent and sum up the scrollLeft values. I will give that a go this morning. |
components/dropdown-menu/index.js
Outdated
| componentDidUpdate( prevProps, prevState ) { | ||
| const { open, activeIndex } = this.state; | ||
|
|
||
| this.calculateMenuPosition(); |
There was a problem hiding this comment.
Noting that this will be called after setting state from calculateMenuPosition, so calculateMenuPosition will always be called twice when it updates.
There was a problem hiding this comment.
I can't think of any way to avoid that. Do you have any suggestions?
There was a problem hiding this comment.
What are the circumstances in which we want to calculate the menu position when the component updates? If it's only when going from unopened to opened, should it go into the condition below?
components/dropdown-menu/index.js
Outdated
| @@ -149,9 +151,33 @@ export class DropdownMenu extends Component { | |||
| } | |||
| } | |||
|
|
|||
| calculateMenuPosition() { | |||
| const { toggle } = this.nodes; | |||
| if ( toggle ) { | |||
There was a problem hiding this comment.
We could avoid nesting the rest of the function by making this an early return instead:
if ( ! toggle ) {
return;
}
I could do that by capturing scroll events but they don't bubble so I'd have to put a listener on everything up to the offsetParent which would very much break out of the sphere of knowledge that a react component is meant to have and also very hard to maintain given the way react could change anything above without telling the component so I suspect that solution would be very controversial. Alternatively I could poll the position every 100 milliseconds but I don't like that solution either. Do you have any suggestions? |
I don't see any great alternatives here. Maybe we should see if it becomes a real problem and decide to address then. If it were the case that the toolbar knew it had DropdownMenu as a child, one option can be to bind a http://andrewhfarmer.com/component-communication/#2-instance-methods |
Allows the menu to be moved when the toggle button is scrolled.
|
I have changed it to recalculate the menu position only when it is opened and then every tenth of a second until it is closed. I don't believe this will adversely effect performance but if it turns out to be a problem it is pretty easy to increase the interval time or simply remove the interval and put up with the problem that the menu will not move if the toolbar is scrolled. As it is currently completely broken in master I'm going to go ahead and merge because this can't be any worse. |
Adding the sliding toolbar in #2443 caused the drop-down menu to stop working because overflow was hidden.
This works around the issue by making the popup menu positioned absolutely to a parent of the sliding div hence avoiding the overflow:none.
This also fixes the styling issue on Firefox due to the changed parent of the slot-fill div causing the display:flex to not be applied.