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.

Sign up for GitHub

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

Jump to bottom

fix(label): RTL fix for label margin #11330

Closed
wants to merge 4 commits into from
Closed

fix(label): RTL fix for label margin #11330

wants to merge 4 commits into from

Conversation

sijav
Copy link
Contributor

@sijav sijav commented Apr 23, 2017

Short description of what this resolves:

RTL fix (scss changes) for label margin for wp, ios and android platform

Changes proposed in this pull request:

  • Add label margin prop for RTL (md, ios)
  • Fix label margin for RTL (md, ios, wp)

Ionic Version: 2.x / 3.x

Fixes: #11211

@sijav
fix(label): RTL fix for label margin
0b49a2f 
RTL fix (scss changes) for label margin for wp, ios and android platform
manucorporat
manucorporat suggested changes Apr 23, 2017
View reviewed changes
Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

@sijav

src/components/label/label.ios.scss Outdated
@@ -6,6 +6,9 @@
/// @prop - Margin of the label
$label-ios-margin: $item-ios-padding-top ($item-ios-padding-right / 2) $item-ios-padding-bottom 0 !default;

/// @prop - Margin of the label for RTL
$label-ios-margin-rtl: $item-ios-padding-top 0 $item-ios-padding-bottom ($item-ios-padding-right / 2) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align the content with the rest of variables?

src/components/label/label.wp.scss Outdated
@@ -17,6 +17,10 @@ $label-wp-text-color-focused: color($colors-wp, primary) !default;
margin: $item-wp-padding-top ($item-wp-padding-right / 2) $item-wp-padding-bottom 0;
}

[dir="rtl"] .label-wp {
margin: $item-wp-padding-top 0 $item-wp-padding-bottom ($item-wp-padding-right / 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we missed the $label-wp-margin-rtl, would you mind adding both?

sijav added 2 commits April 24, 2017 02:00
@sijav
refactor(label): add prop of margin of the label
3741676 
Add prop of margin of the label.
@sijav
refactor(label): align with rest of the variable
388d5b4 
Align the content with rest of the variable
@AmitMY
Copy link
Contributor

AmitMY commented Apr 23, 2017

Looks good to me. @manucorporat ?

manucorporat
manucorporat approved these changes Apr 23, 2017
View reviewed changes
Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Waiting for approval from @brandyscarney

@brandyscarney
Copy link
Member

Is there (or will there be) a PR for the input as well? This PR messes up the alignment for floating labels since the input's margin is incorrect. Screenshot before this PR:

screen shot 2017-04-24 at 12 13 13 pm

After this PR:

screen shot 2017-04-24 at 12 13 04 pm

@AmitMY
Copy link
Contributor

AmitMY commented Apr 24, 2017 •
edited
Loading

@brandyscarney There is something weird here.
Clearly, the margin left is ($item-md-padding-right / 2), but you screenshot shows 0.

Also, no, there is no current input rtl PR, all in due time :)

@brandyscarney
Copy link
Member

@AmitMY This rule is taking precedence so it is getting margin-left: 0

screen shot 2017-04-24 at 12 53 14 pm

@sijav
Copy link
Contributor Author

sijav commented Apr 25, 2017

@brandyscarney I'm doing it now, looks like one pr should be for input and another one for select and I think search-bar must change too ...

@AmitMY
Copy link
Contributor

AmitMY commented Apr 25, 2017

@sijav Take a look at this.
If it will get merged, then we can do the same thing for margin, border, and position, and to fix all of the components together, once. We are still finalising the syntax and changes, but I think this (or some version of it) will be merged soon.

I'm letting you know, just so you don't do extra work that will be rejected if this goes in. I will keep you updated

@sijav
Copy link
Contributor Author

sijav commented Apr 25, 2017 •
edited
Loading

@AmitMY Great job! Thanks for letting me know, but we still need some support for typescript as well (searchbar for ios or sliding item component and so on)

@AmitMY
Copy link
Contributor

AmitMY commented May 13, 2017

@brandyscarney This can be closed

@sijav
Copy link
Contributor Author

sijav commented May 13, 2017

Closing this PR in favor of #11342

@sijav sijav closed this May 13, 2017
@sijav sijav deleted the rtl-fix-label branch May 15, 2017 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manucorporat manucorporat manucorporat approved these changes

@brandyscarney brandyscarney Awaiting requested review from brandyscarney

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sijav @AmitMY @brandyscarney @manucorporat