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(alert-component): RTL fix for alert item's icon #12876

Closed
wants to merge 3 commits into from

Conversation

sijav
Copy link
Contributor

@sijav sijav commented Sep 13, 2017

Short description of what this resolves:

This is a suggestion for RTL fix for icons on alert items (checkboxes
and radios)

Changes proposed in this pull request:

  • add order for RTL in sass to put it at the end

Ionic Version: 3.x

Fixes: #11211

This is a suggestion for RTL fix for icons on alert items (checkboxes
and radios)
@AmitMY
Copy link
Contributor

AmitMY commented Sep 13, 2017

Did you try perhaps not duplicating the element, but using order to move it around?

@sijav
Copy link
Contributor Author

sijav commented Sep 14, 2017

@AmitMY No I did not, I don't know if order can make the icon position correct though...
Maybe we should look into icons?

@AmitMY
Copy link
Contributor

AmitMY commented Sep 14, 2017

How will using icons fix it?
If you can try using order instead it would be great, if not please let me know and I will give it a try

instead of dupplicate icons with *ngIf to check if it's rtl or not, an
order will be used to place them correctly
@sijav
Copy link
Contributor Author

sijav commented Sep 14, 2017

@AmitMY You were right a simple order can make it right instead of duplicate icons however I still am not sure if we're using the same pattern as we used before, can you please check it? thanks.

@AmitMY
Copy link
Contributor

AmitMY commented Sep 14, 2017

Yeah, this is the style for orders (custom properties in general). Thanks for the update

@brandyscarney Can you please take a look at this minor fix?

@sijav
Copy link
Contributor Author

sijav commented Sep 29, 2017

Sry my bad, this doesn't fix anything but also makes it even worse!
I just forgot to run my app with $app-direction: rtl.
Everything seems right about this section.

@brandyscarney
Copy link
Member

Thank you for reviewing this @AmitMY! Just to be clear - this PR is no longer necessary?

@AmitMY
Copy link
Contributor

AmitMY commented Oct 3, 2017

@brandy
No problem.
This one is not necessary, no, but it does bring up a solid use case for #12078

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