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(item-sliding): RTL fix for item sliding #11825

Merged
merged 16 commits into from
Jun 9, 2017

Conversation

sijav
Copy link
Contributor

@sijav sijav commented May 28, 2017

Short description of what this resolves:

this will resolve a but that options would disappear on RTL.

Changes proposed in this pull request:

  • the class will be placed based on site direction

Ionic Version: 3.x

Fixes: #11211

@sijav
Copy link
Contributor Author

sijav commented May 28, 2017

This will complete with #11822

@AmitMY
Copy link
Contributor

AmitMY commented May 28, 2017

I am not sure about doing it from TS.
In theory, we can fix it from scss here by manipulating the order like

@include ltr() { order: 1 }
@include rtl() { order: -1 }

Because:

  • It allows us to change the naming swipe-right to be correct - swipe-end
  • It allows real-time direction change without extra effort (if you initiate the view and then change direction your solution would stay in the first direction)

What do you think?

@sijav
Copy link
Contributor Author

sijav commented May 28, 2017

@AmitMY Yeah well ... this was just a quick fix for this...

@AmitMY
Copy link
Contributor

AmitMY commented May 28, 2017

@sijav Haha not bashing or anything, you are doing great work :)
This would work, yes, but not for real time, and it will be simple to change so I noted it. I just know that in the future it will annoy me such things don't work in real-time (and some things are just a lot of work so we drop them for now)

This can be merged and fixed in the future, but we all know what happens when we say that :P

@sijav
Copy link
Contributor Author

sijav commented May 28, 2017

I know I meant that I didn't think of anything but to announce this bug.

@sijav
Copy link
Contributor Author

sijav commented May 30, 2017

@AmitMY I've changed the fix, I'm using scss changes instead of ts changes... let me know what you think.

@AmitMY
Copy link
Contributor

AmitMY commented May 30, 2017

@sijav Did you test this? it seems very odd to me. Also that you changed the width, and classes that are not related as I think that what needs to be changed is the order, not these classes (which are clones - see #11829)

I believe this selector - "ion-item-sliding.active-swipe-right button[expandable] {" needs a change, like:

@include ltr() { order: 1 }
@include rtl() { order: -1 }

but don't worry about it, as the ltr mixin is not yet available on master..

I think this one should be paused for the moment, and when this - #11635 - gets merged, we should continue discussion

@sijav
Copy link
Contributor Author

sijav commented May 30, 2017

@AmitMY Yes I've tested it and it worked great.
And no there's no order matters it just a matter of left and right class that visible the right property as they being slide...
EDIT: test it with #11822 to work
EDIT: now that I looked at it, it's a mater of expandable buttons I'll test it

@sijav
Copy link
Contributor Author

sijav commented May 30, 2017

@AmitMY this is still buggy but that doesn't have to do with just a simple order the left and right should be for left and right only regardless of being ltr or rtl, I'm working on it and let you know ...
Conclusion: This fix is still under development...

unless we use start or end for rtl and ltr right should be placed right
and so does left.
@sijav
Copy link
Contributor Author

sijav commented May 31, 2017

@AmitMY Could you please review this? this is the only way I could make it work!
Be noticed that now Right will place the sliding options to right side of the item and so does Left will place the sliding option button to the left side of the item and unless we use start or end this is the only correct way to do it.

@include rtl() {
justify-content: flex-start;

&:not([side=right]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these :not([side=right]) are here for default side (when side is not defined in sliding option item) in RTL mode.
In LTR mode there should be positioned on right by default but on RTL they need to be placed left...
In another word when side is not defined it should be placed on end

@AmitMY
Copy link
Contributor

AmitMY commented Jun 7, 2017

Can you please redo this with multi-dir, rtl, and ltr? too much has changed :)

@sijav
Copy link
Contributor Author

sijav commented Jun 8, 2017

@AmitMY done, can you review this?

@@ -17,26 +17,52 @@ ion-item-sliding .item {
}

ion-item-options {
@include position(0, 0, null, null);

// scss-lint:disable PropertySpelling
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 move this comment to inside the mixin?

Copy link
Contributor Author

@sijav sijav Jun 8, 2017

Choose a reason for hiding this comment

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

@AmitMY so I should put it at two places? multi-dir and rtl mixin at the same main selector?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better, yes. Just so it is more specific.

@brandyscarney brandyscarney merged commit 10f4df4 into ionic-team:master Jun 9, 2017
@brandyscarney
Copy link
Member

Thank you! 👍

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.

3 participants