Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add icon margin that works on both LTR & RTL, re-add removed styles #104

Conversation

sheela-svmx
Copy link
Contributor

@sheela-svmx sheela-svmx commented Feb 26, 2019

Issue:

  • The -webkit and -moz fix would repleace margin-left / margin-right depending on the direction of render. But this fix does not work on IE plus unnessary browser prefix in the code.

  • Styles were removed in this PR
    This was causing regression in px-dropdown padding left when it was used in readOnly mode.

Proposed new:

Add margin-right as well , so icon will have margin-right and margin-left, so irrespective of the rendering, text would not be too close to the icon

Re-add back the removed styles

screen shot 2019-03-05 at 10 40 32 am

screen shot 2019-03-05 at 10 52 42 am

screen shot 2019-03-05 at 10 41 10 am

So all icons will have margin left and margin right, so we dont need browser prefix fixes plus works for IE
@cla-bot cla-bot bot added the cla-signed label Feb 26, 2019
@sheela-svmx
Copy link
Contributor Author

*/
-webkit-margin-start: $inuit-base-spacing-unit--tiny;
-moz-margin-start: $inuit-base-spacing-unit--tiny;
margin-right: $inuit-base-spacing-unit--tiny;
Copy link
Contributor

@benjaminliugang benjaminliugang Feb 26, 2019

Choose a reason for hiding this comment

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

This fix only can add margin-right value for the dropdown button's icon, the item icon's space issue still exist.
image

We should add margin-right value for item-icon, but if we only add margin-right, for RTL, the item-icon's space issue will comes out again.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@joshsylvester joshsylvester left a comment

Choose a reason for hiding this comment

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

Can we include screenshots of how these changes affect the component?

HISTORY.md Outdated
@@ -1,3 +1,7 @@
# v4.9.3

- Use margin right and left to avoid browser specif prefix fix for rtl space issue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Use margin right and left to avoid browser specif prefix fix for rtl space issue
- Use margin right and left to avoid browser specific prefix fix for rtl space issue

@evanjd
Copy link
Contributor

evanjd commented Mar 5, 2019

Also - just repeating again my strong preference for not committing version bumps and HISTORY.md updates in PRs - as both this PR and #105 will create conflicts for each other.

Also, judging from the comments, both this and PR #105 treat the same problem? They should probably be part of a single version bump rather than released individually.

@sheela-svmx
Copy link
Contributor Author

@evanjd Agreed I will combine the 2 PR into 1 so there will be one version change

@sheela-svmx sheela-svmx changed the title add margin right UI-3235 and I-3236 add margin right + re-add styles back Mar 5, 2019
@sheela-svmx
Copy link
Contributor Author

@evanjd updated ur comments and combined into one PR. will close the other PR.

@joshsylvester joshsylvester changed the title UI-3235 and I-3236 add margin right + re-add styles back Add icon margin that works on both LTR & RTL, re-add removed styles Mar 5, 2019
@joshsylvester joshsylvester merged commit 556d24b into predixdesignsystem:master Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants