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

fix(rtl): add icon-start and icon-end notations #11217

Closed
wants to merge 52 commits into from

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Apr 13, 2017

TO BE MERGED AFTER #11342

Short description of what this resolves:

icons position on rtl

Changes proposed in this pull request:

  • add icon-start and icon-end notations
  • deprecate icon-left and icon-right
  • replace in all core ionic files usage of icon-left and icon-right

Why not fully support icon-left and icon-right?

Because it is a css styling only, there will need to be use of float or dir which are both undesired, so the only remaining option is to deprecate the old way

Ionic Version: 3.x

@jlucfarias
Copy link

jlucfarias commented Apr 17, 2017

This is a good ideia but if someone need to set the icon always in left/right? I think that icon-left and icon-right should be keeped

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 17, 2017

@John-Luke I kept them exactly the same as their current bahvior. (which is not that great)
Do you think it will be OK to use float in this case? I just hate floating stuff and want another opinion before using it.

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 21, 2017

@brandyscarney Anything to fix here?

@brandyscarney
Copy link
Member

@AmitMY So this is renaming icon-left to icon-start, icon-right to icon-end but it isn't a breaking change since the CSS styling to support the old way is still there. It looks good to me.

I don't think you need to introduce rearranging the icons using icon-left or icon-right in this PR. I'm sure it could be improved upon, but this is a good start. CC @manucorporat for approval.

@@ -161,7 +161,7 @@ $alert-md-radio-border-color-on: $alert-md-button-text-color !defau
$alert-md-radio-icon-top: 2px !default;

/// @prop - Left of the icon in the alert radio
$alert-md-radio-icon-left: 2px !default;
$alert-md-radio-icon-start: 2px !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

@AmitMY this variable value is not longer aligned with the rest of scss variables. the same applies many times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manucorporat You have the eyes like a hawk :)
Fixed

/// @prop - Transform for the active tab button icon when the layout is icon-left
$tabs-md-tab-icon-left-transform-active: translate3d(-2px, 0, 0) !default;
/// @prop - Transform for the active tab button icon when the layout is icon-start
$tabs-md-tab-icon-start-transform-active: translate3d(-2px, 0, 0) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here! it is not aligned hahha sorry :D

and I think this is breaking change too, we want to avoid them:
what if we do:

// deprecated
$alert-left-label:            7px !default;
/// @prop - 
$alert-start-label:          $alert-left-label !default;

alert-label {
  left: $alert-start-label;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure we are looking at the same thing..
I did not change 'alert-left-label' at all.. this will come in a future RTL-alert PR, in some time

I will fix the alignment in about an hour. Anything else u can spot?

Copy link
Member

Choose a reason for hiding this comment

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

@AmitMY Sorry he was just using that as an example variable. So basically if we rename:

$tabs-md-tab-icon-left-transform-active to $tabs-md-tab-icon-start-transform-active

(or any other variables) it will be considered a breaking change if any users are overriding these variables. It's an easy fix, sure, but it's still technically breaking and would prevent us from merging this until the 4.0 release. We're suggesting doing the following:

// deprecated
$tabs-md-tab-icon-left-transform-active:            translate3d(-2px, 0, 0) !default;

/// @prop - 
$tabs-md-tab-icon-start-transform-active:           $tabs-md-tab-icon-left-transform-active !default;

in-the-selector-the-variable-is-used {
  transform: $tabs-md-tab-icon-start-transform-active;
}

This would introduce *-start as the new variable (and it would be documented) but if they are using the *-left variable to override styles it would still be used. Does that make sense? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok.
I fixed the bad spacing, and added the deprecated variables. Anything else?

@AmitMY
Copy link
Contributor Author

AmitMY commented Apr 24, 2017

@manucorporat Anything else/approve?

AmitMY added 27 commits April 25, 2017 22:43
# Conflicts:
#	src/components/label/label.ios.scss
# Conflicts:
#	src/components/range/range.ios.scss
#	src/components/range/range.md.scss
#	src/components/range/range.wp.scss
#	src/components/slides/slides.scss
# Conflicts:
#	src/components/action-sheet/action-sheet-component.ts
#	src/components/alert/alert.ios.scss
#	src/components/alert/alert.md.scss
#	src/components/alert/alert.wp.scss
#	src/components/tabs/tabs.scss
#	src/components/toolbar/toolbar.ios.scss
#	src/components/toolbar/toolbar.md.scss
#	src/components/toolbar/toolbar.wp.scss
# Conflicts:
#	src/components/action-sheet/action-sheet-component.ts
#	src/components/alert/alert.ios.scss
#	src/components/alert/alert.md.scss
#	src/components/alert/alert.wp.scss
#	src/components/tabs/tabs.scss
#	src/components/toolbar/toolbar.ios.scss
#	src/components/toolbar/toolbar.md.scss
#	src/components/toolbar/toolbar.wp.scss
@AmitMY
Copy link
Contributor Author

AmitMY commented May 20, 2017

Closing in favor of #11737

@AmitMY AmitMY closed this May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants