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

fix(docs): changing component variables example snippet#1626

Merged
lucivpav merged 16 commits intomicrosoft:masterfrom
lucivpav:docs/theming-examples-changing-component-variables-fix
Jul 30, 2019
Merged

fix(docs): changing component variables example snippet#1626
lucivpav merged 16 commits intomicrosoft:masterfrom
lucivpav:docs/theming-examples-changing-component-variables-fix

Conversation

@lucivpav
Copy link
Contributor

This PR updates the outdated code snippets in changing component variables section of docs. The buttons were not rendered as a user would expect. Relevant issue with error description: #1592.

Before:
image

After:
image

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #1626 into master will decrease coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1626      +/-   ##
==========================================
- Coverage   71.49%   71.28%   -0.21%     
==========================================
  Files         846      846              
  Lines        6945     6938       -7     
  Branches     2002     1982      -20     
==========================================
- Hits         4965     4946      -19     
- Misses       1974     1986      +12     
  Partials        6        6
Impacted Files Coverage Δ
...ages/react/test/specs/behaviors/testDefinitions.ts 87.39% <0%> (-5.58%) ⬇️
...b/accessibility/Behaviors/Tree/treeItemBehavior.ts 100% <0%> (ø) ⬆️
packages/react/src/components/Tree/TreeItem.tsx 76.31% <0%> (+1.31%) ⬆️

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 7dc2027...03e54a9. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #1626 into master will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1626      +/-   ##
==========================================
+ Coverage   70.97%   71.24%   +0.26%     
==========================================
  Files         860      851       -9     
  Lines        7153     7035     -118     
  Branches     2078     2027      -51     
==========================================
- Hits         5077     5012      -65     
+ Misses       2070     2017      -53     
  Partials        6        6
Impacted Files Coverage Δ
...mes/teams/components/Checkbox/checkboxVariables.ts 0% <0%> (-75%) ⬇️
packages/react/src/components/Chat/Chat.tsx 62.5% <0%> (-8.93%) ⬇️
packages/react/src/components/Dialog/Dialog.tsx 31.57% <0%> (-3.31%) ⬇️
...ackages/react/src/components/Checkbox/Checkbox.tsx 80% <0%> (-1.82%) ⬇️
packages/react-proptypes/src/index.ts 34.26% <0%> (-0.46%) ⬇️
packages/react/src/lib/factories.ts 94.2% <0%> (-0.25%) ⬇️
...ages/react/test/specs/behaviors/testDefinitions.ts 92.97% <0%> (-0.12%) ⬇️
...es/teams/components/ItemLayout/itemLayoutStyles.ts 20% <0%> (ø) ⬆️
packages/react/src/components/Button/Button.tsx 80% <0%> (ø) ⬆️
...ges/react/src/components/Attachment/Attachment.tsx 77.77% <0%> (ø) ⬆️
... and 56 more

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 fbb59d8...7f0d856. Read the comment docs.

paddingLeftRightValue: 20,
}}
/>
<Button
Copy link
Contributor

@kuzhelov kuzhelov Jul 19, 2019

Choose a reason for hiding this comment

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

unfortunately, this leads to change in the example's semantics. Originally the purpose was to demo variables usage - specifically, how it is possible to customize variables and obtain different look of the component. Now what we have is the styles demo, with additional aspect of using variables as style parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked to Roman about it - we should advertise the colors from the color palette rather than the usage of the styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alinais do you mean, for example, siteVars.colors.pink[50] instead of, say, pink? I have updated the PR, please let me know whether you like it now :)

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked 📚 docs and removed needs author feedback Author's opinion is asked labels Jul 19, 2019
lucivpav added 2 commits July 26, 2019 11:42
… of github.com:lucivpav/react into docs/theming-examples-changing-component-variables-fix
@vercel vercel bot temporarily deployed to staging July 26, 2019 09:51 Inactive
@lucivpav
Copy link
Contributor Author

The example looks like this now:
image

@lucivpav
Copy link
Contributor Author

Please note that only a small fraction of icons support the color variable. User thus may be confused, why his icon does not have a proper color. Relevant issue #1039.

@vercel vercel bot temporarily deployed to staging July 30, 2019 08:55 Inactive
@kuzhelov kuzhelov added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Jul 30, 2019
@lucivpav lucivpav merged commit d180879 into microsoft:master Jul 30, 2019
@lucivpav lucivpav deleted the docs/theming-examples-changing-component-variables-fix branch July 30, 2019 09:27
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.

4 participants