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(select): RTL fix for searchbar #11355

Merged
merged 7 commits into from
May 17, 2017
Merged

fix(select): RTL fix for searchbar #11355

merged 7 commits into from
May 17, 2017

Conversation

sijav
Copy link
Contributor

@sijav sijav commented Apr 25, 2017 •
edited
Loading

Short description of what this resolves:

RTL fix for searchbar component

Changes proposed in this pull request:

  • change margin and padding dynamically based on app's direction

Ionic Version: 2.x / 3.x

Fixes: #11211

sijav added 2 commits April 25, 2017 14:37
@sijav
fix(select): RTL fix for searchbar
9853eb0 
RTL fix for searchbar component
@sijav
fix bad reference
a4d679b 
I forgot to refrence _isRTL
AmitMY
AmitMY reviewed Apr 25, 2017
View reviewed changes
src/components/searchbar/searchbar.ts Outdated
@@ -63,6 +63,7 @@ export class Searchbar extends BaseInput<string> {
_isActive: boolean = false;
_showCancelButton: boolean = false;
_animated: boolean = false;
_isRTL: boolean = this._plt.isRTL();
Copy link
Contributor

Choose a reason for hiding this comment

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

isRTL is now a variable of platform, and not a method. so instead of having, this line, and then if (this._isRTL), use:
if (this._plt.isRTL)

It will then also work on realtime direction switch.

Nice work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a bunch, didn't know that.

@manucorporat
Copy link
Contributor

@sijav you should be able to run gulp validate before submitting a PR, I can see several styling issues

@sijav
Copy link
Contributor Author

sijav commented Apr 25, 2017 •
edited
Loading

@manucorporat could you address me those issues? with gulp validate and gulp lint.scss the only thing I get is Fatal undefined and nothing else, and I can't seem to find the problem.
EDIT: find out that I haven't install ruby yet :D
EDIT: I'm still getting the same result even though I've installed ruby version 2.3.3p222 (2016-11-12 rev 56859) [x64-mingw32]

@AmitMY
Copy link
Contributor

AmitMY commented May 5, 2017

@sijav Can you please remove all scss changes and only leave the ts one?
This PR - #11342 - covers the scss, and is about done

@AmitMY
Copy link
Contributor

AmitMY commented May 13, 2017

@brandyscarney Tested, works as expected on ltr & rtl (after applying this fix - #11651 on master)

(gulp e2e.watch --folder=searchbar/basic)

@brandyscarney brandyscarney merged commit ca71072 into ionic-team:master May 17, 2017
@brandyscarney
Copy link
Member

brandyscarney commented May 17, 2017 •
edited
Loading

Thanks for the PR @sijav, and thanks for reviewing @AmitMY. Looks good!

@sijav sijav deleted the rtl-fix-searchbar branch May 30, 2017 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AmitMY AmitMY AmitMY left review comments

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 @manucorporat @AmitMY @brandyscarney