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

feat(icon): adding xSpacing=['none'|'before'|'after'|'both'] prop to Icon #22

Merged
merged 1 commit into from Jul 30, 2018

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Jul 30, 2018

Icon

This PR will introduce possibility to specify xSpacing=['none'|'before'|'after'|'both'] prop on an Icon component in order to adjust horizontal space around icons.

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

xSpacing

An icon can achieve the following horizontal spacing:

  • xSpacing='none': no space to before or after the icon;
  • xSpacing='before': space before the icon;
  • xSpacing='after': space after the icon;
  • xSpacing='both': space before and afer the icon;

screen shot 2018-07-30 at 16 32 13

<div>
  <Icon xSpacing="none" name="help" />
  <Icon xSpacing="before" name="help" />
  <Icon xSpacing="after" name="help" />
  <Icon xSpacing="both" name="help" />
</div>

generates:

<div>
  <i class="ui-icon"></i>
  <i class="ui-icon"></i>
  <i class="ui-icon"></i>
  <i class="ui-icon"></i>
</div>

@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 30, 2018

@mnajdova @kuzhelov @levithomason
ready for review

@miroslavstastny
Copy link
Member

Not sure about using Left and Right as these become right and left in RTL. When I talked with @levithomason about that he proposed some universal words which I can't remember (before/after?).
cc @jurokapsiar

@bmdalex bmdalex changed the title feat(icon): adding 'spaceLeft', 'spaceRight' and 'fitted' props to Icon feat(icon): adding 'xSpacing=['none'|'left'|'right'|'both']' prop to Icon Jul 30, 2018
@codecov
Copy link

codecov bot commented Jul 30, 2018

Codecov Report

Merging #22 into master will decrease coverage by 0.7%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   84.28%   83.58%   -0.71%     
==========================================
  Files          59       59              
  Lines         789      804      +15     
  Branches      159      163       +4     
==========================================
+ Hits          665      672       +7     
- Misses        120      128       +8     
  Partials        4        4
Impacted Files Coverage Δ
src/components/Avatar/avatarRules.ts 83.33% <ø> (ø) ⬆️
src/components/Icon/Icon.tsx 100% <100%> (ø) ⬆️
src/components/Icon/iconVariables.ts 100% <100%> (ø) ⬆️
src/styles/customCSS.ts 100% <100%> (ø) ⬆️
src/components/Icon/iconRules.ts 70.58% <50%> (-20.33%) ⬇️

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 4d2d0a8...49f46ca. Read the comment docs.

@bmdalex bmdalex changed the title feat(icon): adding 'xSpacing=['none'|'left'|'right'|'both']' prop to Icon feat(icon): adding xSpacing=['none'|'left'|'right'|'both'] prop to Icon Jul 30, 2018
@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 30, 2018

@miroslavstastny thx for sharing this; once @levithomason gets back we can definitely amend the naming; updated proposal after discussing with @mnajdova and Hugh:
xSpacing=['none'|'left'|'right'|'both']
screen shot 2018-07-30 at 16 00 20
fyi @jurokapsiar

@mnajdova
Copy link
Contributor

The naming convention for this prop is really good, we can reuse this for the box model in the theming. I agree with @miroslavstastny for replacing the left and right with before and after, because that way we can be consistent for the potential adding of the ySpacing in the future. Replace them and we are good to merge this PR in order to unblock the other PRs and address potential changes after the theme PR is merged.

@bmdalex
Copy link
Collaborator Author

bmdalex commented Jul 30, 2018

ok, updated one more time after discussing with @miroslavstastny and @mnajdova ; thx guys:
current API proposal:

xSpacing=['none'|'before'|'after'|'both']

screen shot 2018-07-30 at 16 32 13

@bmdalex bmdalex changed the title feat(icon): adding xSpacing=['none'|'left'|'right'|'both'] prop to Icon feat(icon): adding xSpacing=['none'|'before'|'after'|'both'] prop to Icon Jul 30, 2018
Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Good to be merged here in order to unblock the other PRSs. We will return to this when implementing the box model for the theming. Thanks for the changes!

@levithomason
Copy link
Member

Proposing we use fitted and start/end for this use case. See #39 for details and conversation.

<Icon fitted />
<Icon fitted='start' />
<Icon fitted='end' />

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

4 participants