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

refactor(item): replaced attributes with item-start / item-end #11125

Merged
merged 10 commits into from May 5, 2017

Conversation

Projects
None yet
3 participants
@AmitMY
Member

AmitMY commented Apr 7, 2017

Short description of what this resolves:

Ambiguity in RTL projects.
Issue:
#11124

Changes proposed in this pull request:

  • Change item-left to item-start
  • Change item-right to item-end
  • Deprecated SCSS variables with -item-left and created -item-start. same for right/end

Ionic Version: 3.1.x

refactor(item): replaced item-left with item-start
replaced item-right with item-end
@manucorporat

This comment has been minimized.

Show comment
Hide comment
@manucorporat

manucorporat Apr 8, 2017

Member

Well, we can easily make this a non-breaking change.

  • Let's add item-left and item-right back (as a deprecated util)
  • We could add some logic to Item, so it prints a warning message when people use item-right or item-left.
Member

manucorporat commented Apr 8, 2017

Well, we can easily make this a non-breaking change.

  • Let's add item-left and item-right back (as a deprecated util)
  • We could add some logic to Item, so it prints a warning message when people use item-right or item-left.
@AmitMY

This comment has been minimized.

Show comment
Hide comment
@AmitMY

AmitMY Apr 8, 2017

Member

@manucorporat That is possible, although, I have no idea what to do with:
https://github.com/AmitMY/ionic/blob/cb5ac7cfc1e2a9d49c2a696c120b37e4e3934500/src/components/item/item.ts#L277
https://github.com/AmitMY/ionic/blob/cb5ac7cfc1e2a9d49c2a696c120b37e4e3934500/src/components/item/item.ts#286

And where exactly to add the deprecation message.

The other thing for support, is just the 16 .scss files, which are easy fixes.

Do I also need to add /** @deprecated **/ above .scss selectors/variables that are deprecated? because I can't make someone not use the old .scss variables.

Member

AmitMY commented Apr 8, 2017

@manucorporat That is possible, although, I have no idea what to do with:
https://github.com/AmitMY/ionic/blob/cb5ac7cfc1e2a9d49c2a696c120b37e4e3934500/src/components/item/item.ts#L277
https://github.com/AmitMY/ionic/blob/cb5ac7cfc1e2a9d49c2a696c120b37e4e3934500/src/components/item/item.ts#286

And where exactly to add the deprecation message.

The other thing for support, is just the 16 .scss files, which are easy fixes.

Do I also need to add /** @deprecated **/ above .scss selectors/variables that are deprecated? because I can't make someone not use the old .scss variables.

@brandyscarney brandyscarney added the rtl label Apr 13, 2017

@brandyscarney

This comment has been minimized.

Show comment
Hide comment
@brandyscarney

brandyscarney Apr 13, 2017

Member

I think this should behave the same everywhere:

left - Always left regardless of dir
right - Always right regardless of dir
start - The same as left if direction is left-to-right and right if direction is right-to-left.
end - The same as right if direction is left-to-right and left if direction is right-to-left.

This would make it so there are no breaking changes, and it improves support.

Member

brandyscarney commented Apr 13, 2017

I think this should behave the same everywhere:

left - Always left regardless of dir
right - Always right regardless of dir
start - The same as left if direction is left-to-right and right if direction is right-to-left.
end - The same as right if direction is left-to-right and left if direction is right-to-left.

This would make it so there are no breaking changes, and it improves support.

@AmitMY

This comment has been minimized.

Show comment
Hide comment
@AmitMY

AmitMY Apr 13, 2017

Member

@brandyscarney While I agree with you on text-X, icon-X, side for menu and for sliding item, I disagree on this one.
The item's left and right are very strict things as far as I see them (currently, left is a separate container, while right is inside the main container etc, and those are not just rules)

On this one, I really believe there should be a deprecation for left and right.

Side note: I've seen you reviewed most rtl PRs. Can you please review this one as well? #11121

Member

AmitMY commented Apr 13, 2017

@brandyscarney While I agree with you on text-X, icon-X, side for menu and for sliding item, I disagree on this one.
The item's left and right are very strict things as far as I see them (currently, left is a separate container, while right is inside the main container etc, and those are not just rules)

On this one, I really believe there should be a deprecation for left and right.

Side note: I've seen you reviewed most rtl PRs. Can you please review this one as well? #11121

@brandyscarney brandyscarney merged commit 26c653e into ionic-team:master May 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment