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

fix(Theme): Deep merge variables #1907

Merged
merged 15 commits into from
Sep 12, 2019
Merged

Conversation

miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Sep 10, 2019

Do deep merge of site variables and component variables.

Motivation

Let's have two themes (parent, child) merged.
To resolve styles, follow steps occur

  1. child.siteVariables are merged on top of parent.siteVariables to compute siteVariables
  2. computed siteVariables are used to compute parent.componentVariables and child.componentVariables, child.componentVariables are merged on top of parent.componentVariables
  3. computed componentVariables are used to compute parent.styles and child.styles

That means that if child variables remove a variable from parent and parent styles use that variable, an exception will be thrown.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7ca52d9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1907   +/-   ##
========================================
  Coverage          ?   70.5%           
========================================
  Files             ?     884           
  Lines             ?    7794           
  Branches          ?    2254           
========================================
  Hits              ?    5495           
  Misses            ?    2289           
  Partials          ?      10
Impacted Files Coverage Δ
packages/react/src/lib/mergeThemes.ts 100% <100%> (ø)
packages/react/src/lib/deepmerge.ts 100% <100%> (ø)

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 7ca52d9...4468e48. Read the comment docs.

@layershifter
Copy link
Member

Please measure it before merging 🤔

@miroslavstastny
Copy link
Member Author

Please measure it before merging 🤔

master:

Example Avg Avg diff Avg Normalized Avg Normalized diff Median Median diff Median Normalized Median Normalized diff
ChatWithPopover.perf.tsx 561.7 +3.84% 955.3 955.3 614.07 +27.37% 571.12 571.12
Chat.perf.tsx 540.91 540.91 956.14 +0.09% 482.11 482.11 1164 +103.81%

deep merge branch:

Example Avg Avg diff Avg Normalized Avg Normalized diff Median Median diff Median Normalized Median Normalized diff
Chat.perf.tsx 511.91 511.91 969.01 969.01 482.27 482.27 1122.21 +74.7%
ChatWithPopover.perf.tsx 558.12 +9.03% 996.02 +2.79% 609.04 +26.29% 642.37 642.37

My interpretation of the results: there is no measurable perf impact.

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@miroslavstastny miroslavstastny merged commit 51f1573 into master Sep 12, 2019
@miroslavstastny miroslavstastny deleted the fix/deep-merge-site-vars branch September 12, 2019 20:13
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.

None yet

3 participants