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

feat(Header): add descriptionColor variable #78

Merged
merged 12 commits into from
Aug 23, 2018

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Aug 9, 2018

Header - descriptionColor variables

Sets color of Header.Description.

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

descriptionColor

image

<Header ... description='...' variables={{ descriptionColor: 'red' }} />

@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   88.35%   88.37%   +0.01%     
==========================================
  Files          47       47              
  Lines         773      774       +1     
  Branches      100      101       +1     
==========================================
+ Hits          683      684       +1     
  Misses         87       87              
  Partials        3        3
Impacted Files Coverage Δ
src/components/Header/Header.tsx 95% <100%> (+0.26%) ⬆️

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 e1692aa...e91536a. Read the comment docs.

@levithomason
Copy link
Member

Regarding Layout, you could just use flex-box styling for now if you are blocked. We can add the abstraction later as we find the correct pattern.

@kuzhelov kuzhelov changed the title feat(Header): add descriptionColor variable [WIP] feat(Header): add descriptionColor variable Aug 10, 2018
@kuzhelov kuzhelov changed the title [WIP] feat(Header): add descriptionColor variable feat(Header): add descriptionColor variable Aug 13, 2018
@kuzhelov
Copy link
Contributor Author

new approach to theming is merged with this PR - please, take another look
@levithomason @mnajdova

{
generateKey: false,
},
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified with the following:

    const descriptionElement = HeaderDescription.create(
      descriptionContentOrProps,
      {
        defaultProps: {
          variables: {
            ...(v.descriptionColor && { color: v.descriptionColor }),
          },
        },
        generateKey: false,
      },
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thing that I am worrying about is that this is semantically not the same as providing these custom variables as variables property - here, with the suggested approach, those are passed as defaultProps. Shouldn't we instead consider overrideProps from this config object at least?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can add them in the overrideProps, my comment was just regarding removing the parsing of the descriptionProp considering it can be avoided with the proposed code. If there is no other way, then it can stay like this.

{
generateKey: false,
},
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can add them in the overrideProps, my comment was just regarding removing the parsing of the descriptionProp considering it can be avoided with the proposed code. If there is no other way, then it can stay like this.

@kuzhelov kuzhelov merged commit 5e5d893 into master Aug 23, 2018
@kuzhelov kuzhelov deleted the feat/add-header-description-color branch August 29, 2018 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants