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

feat(menu): Inherit Menu variables in MenuItem #94

Merged
merged 4 commits into from
Aug 16, 2018

Conversation

miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Aug 15, 2018

This is an alternative solution to #84.

For discussion

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

API Proposal

Approach suggested by @kuzhelov in #84 - to pass all the resolved variables in Menu.renderComponent() to the MenuItem. Works great for shorthand, but breaks with child API (currently, there is no way how parent could pass the variables overrides to children when child API is used).

Alternative to child API which solves it:

<Menu renderItems={(variables) => (<MenuItem variables={variables} .../>) .../>

For now we decided to remove Children API support for Menu so this should not be a problem.

@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #94   +/-   ##
=======================================
  Coverage   86.27%   86.27%           
=======================================
  Files          39       39           
  Lines         663      663           
  Branches       99       99           
=======================================
  Hits          572      572           
  Misses         88       88           
  Partials        3        3
Impacted Files Coverage Δ
src/components/Menu/Menu.tsx 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 985eb15...cfdb39b. Read the comment docs.

@@ -78,7 +87,7 @@ const renderComponent = <P extends {}>(

// Resolve variables for this component, allow props.variables to override
const resolvedVariables: ComponentVariablesObject = mergeComponentVariables(
componentVariables[displayName],
componentVariables[useVariablesFrom],
Copy link
Contributor

@kuzhelov kuzhelov Aug 15, 2018

Choose a reason for hiding this comment

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

would it still work for other components with this change - for those where this property is not specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

#70    useVariablesFrom = displayName,

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks!

But, actually, in a shed of recent decision on removing Children API from scope - do we really need to introduce this additional useVariablesFrom mechanism? What if we will do the following instead (as we've discussed it today's noon):

// in Menu.tsx, pseudo-code
renderComponent({..., variables, ... }) => (
      ....
     <MenuItem variables={variables} />
)

When set of cases has reduced to shorthand API those two approaches are absolutely equal in terms of cases covered - while, at the same time,

  • the later one utilizes only those mechanisms that were already introduced
  • semantically both reduce to passing Menu variables to Menu.Item

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally agree. I will remove child API for MenuItem (for now), it is already broken anyway, and pass computed variables to MenuItem in Menu.renderComponent()

@kuzhelov kuzhelov merged commit c9b9d6e into master Aug 16, 2018
@levithomason levithomason deleted the feat/variables-inheritance branch August 5, 2019 14:34
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

2 participants