Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Toolbar): overflow menu via DOM #2010

Merged
merged 44 commits into from
Oct 9, 2019
Merged

Conversation

miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Oct 4, 2019

BREAKING CHANGE

Legacy support for Toolbar overflow using onReduceItems has been removed.

Refactor to the new overflow implementation for supported scenarios or reimplement the logic on client side for use cases which are not covered.

Motivation (old approach problems)

onReduceItems implementation was generic allowing number of edge-case scenarios:

  • it was possible to render custom overflow item anywhere in the list, the overflow item could change its size
  • it was possible to move items to overflow menu in any order, or even multiple items at a time
    The problem with the approach was that to show Toolbar it was necessary to render it multiple times which took significant amount of time (hundreds of ms).

New approach

The new implementation takes vanilla DOM/CSS approach.

  1. All toolbar items including the overflow menu are rendered inside an additional overflow container with overflow: hidden.
  2. After render (or window resize) all direct DOM children of the overflow container are measured. If some children do not fit:
    • these items are hidden.
    • overflow menu is absolutely positioned and displayed.
    • (this is done using CSS manipulation by applying inline styles, no React rerender)
  3. When Toolbar is rerendered with overflow menu open, it calls a callback back to to its parent (with index of the first overflow item). Parent returns list of overflow items in a format suitable for a menu.

New approach limitations

  • Consumer cannot define own reduce logic.
  • Each toolbar item must render exactly one root DOM element (10 toolbar items must render 10 DOM elements in the overflow container).
  • The root DOM element must have the dimensions representing the size of the item.
  • Any toolbar item CANNOT resize itself - To resize a toolbar item, rerender the whole Toolbar
  • Overflow item cannot hold its own state to open/close the overflow menu. The technical reason for that is that to open the overflow menu, the whole Toolbar must be rerendered to remove overflow items from the main area and render them inside the overflow. Symmetrically, when closing the menu, the Toolbar must be rerendered to render all items directly in the main area (root DOM element).
  • There is no support for overflow item to be always displayed (can be added later)
  • There is no support for items being rendered always in the overflow menu (can be added later)

Known issues

  • ToolbarItem with popup set to inline breaks the contract as it renders two sibling elements inside the root DOM element.
  • Overflow item can be displayed out of its container during resize.
  • Overflow item's margins are ignored in the computation (to be fixed later).
  • Overflow menu can be incorrectly positioned in Portal window (general Stardust issue with Popper).

Fixes #1935, fixes #1893, fixes #1885.

@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #2010 into master will decrease coverage by 0.9%.
The diff coverage is 23.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2010      +/-   ##
=========================================
- Coverage   76.71%   75.8%   -0.91%     
=========================================
  Files         160     160              
  Lines        5493    5572      +79     
  Branches     1586    1631      +45     
=========================================
+ Hits         4214    4224      +10     
- Misses       1266    1334      +68     
- Partials       13      14       +1
Impacted Files Coverage Δ
...kages/react/src/components/Toolbar/ToolbarItem.tsx 45.16% <ø> (ø) ⬆️
packages/react-component-ref/src/toRefObject.ts 100% <100%> (ø) ⬆️
packages/react/src/components/Toolbar/Toolbar.tsx 28.76% <22.13%> (-19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5796d9e...5572886. Read the comment docs.

@vercel vercel bot temporarily deployed to staging October 9, 2019 14:08 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolbar overflow prop Toolbar overflow performance Toolbar: overflow is not working in child window
3 participants