Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add mergeMargin() function #114

Merged
merged 3 commits into from
Mar 7, 2019
Merged

feat: add mergeMargin() function #114

merged 3 commits into from
Mar 7, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Mar 6, 2019

🏆 Enhancements

  • Add mergeMargin() function for combining two margins.

@kristw kristw requested a review from a team as a code owner March 6, 2019 20:09
@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #114 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #114   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          73     74    +1     
  Lines         891    898    +7     
  Branches      205    211    +6     
=====================================
+ Hits          891    898    +7
Impacted Files Coverage Δ
packages/superset-ui-dimension/src/mergeMargin.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 a2504f7...6ec9979. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

lgtm 👍 one small suggestion

const { top = 0, left = 0, bottom = 0, right = 0 } = margin1;
const { top: top2 = 0, left: left2 = 0, bottom: bottom2 = 0, right: right2 = 0 } = margin2;

return mode === 'expand'
Copy link
Contributor

@williaster williaster Mar 6, 2019

Choose a reason for hiding this comment

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

you could simplify this a bit

const func = mode === 'expand' ? Math.max : Math.min;

return {
  bottom: func(bottom, bottom2),
  ...repeat
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

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.

2 participants