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

fix(MenuItem): Merge Menu and MenuItem variables #1447

Merged
merged 7 commits into from
Jun 12, 2019

Conversation

miroslavstastny
Copy link
Member

Problem description

Menu and all its MenuItems share the same variables.
Menu passes its variables down to MenuItem as defaultProps. That means that if a consumer defines a variable directly on a MenuItem, it overwrites the variables passed from the Menu:

<Menu items={[
  {content: 'valid item'}, // ok, variables passed from Menu to the item
  {content: 'item', variables: {special: true}} // BUG, item variables replaces Menu variables, this menu item has broken styles
]} />

Solution

Instead of Menu passing its variables via defaultProps, it now does it through overrideProps and shallow-merges Menu variables with MenuItem variables.

  • changelog

Fixes #1392.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #1447 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1447      +/-   ##
==========================================
+ Coverage   73.27%   73.29%   +0.01%     
==========================================
  Files         802      802              
  Lines        6025     6028       +3     
  Branches     1775     1775              
==========================================
+ Hits         4415     4418       +3     
  Misses       1604     1604              
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/components/Menu/Menu.tsx 71.05% <100%> (+2.48%) ⬆️

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 f117326...92f612b. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #1447 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1447      +/-   ##
==========================================
+ Coverage   73.46%   73.67%   +0.21%     
==========================================
  Files         807      807              
  Lines        6086     6089       +3     
  Branches     1772     1753      -19     
==========================================
+ Hits         4471     4486      +15     
+ Misses       1610     1598      -12     
  Partials        5        5
Impacted Files Coverage Δ
...react/src/components/Toolbar/ToolbarRadioGroup.tsx 100% <ø> (+30.76%) ⬆️
packages/react/src/components/Toolbar/Toolbar.tsx 100% <ø> (+47.05%) ⬆️
packages/react/src/components/Menu/Menu.tsx 71.05% <100%> (+2.48%) ⬆️

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 eeb3825...7afaacb. Read the comment docs.

@miroslavstastny miroslavstastny merged commit 1c67f69 into master Jun 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/menu-item-variable-overrides branch June 12, 2019 08:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu items as object array with variables loses correct styling
4 participants