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

ion-select with no-padding attribute in RTL mode not working #11914

Closed
snirs90 opened this issue Jun 4, 2017 · 4 comments
Closed

ion-select with no-padding attribute in RTL mode not working #11914

snirs90 opened this issue Jun 4, 2017 · 4 comments
Assignees

Comments

@snirs90
Copy link
Contributor

snirs90 commented Jun 4, 2017

Ionic version: (check one with "x")
[ ] 1.x (For Ionic 1.x issues, please use https://github.com/ionic-team/ionic-v1)
[ ] 2.x
[x] 3.x

I'm submitting a ... (check one with "x")
[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior:
ion-select with no-padding attribute in RTL mode are not reset the padding.
it is overrided by the css class:

[dir="rtl"] .select-md {
    padding-right: 16px;
    padding-left: 8px;
}

Expected behavior:
no-padding should override with reset the padding properties.

Steps to reproduce:

  1. make the app RTL mode.
  2. add ion-select
  3. put no-padding attribute
  4. The padding is not getting reset and get overrides by [dir="rtl"] .select-md css class.

Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

global packages:

@ionic/cli-utils : 1.3.0
Cordova CLI      : 7.0.1 
Ionic CLI        : 3.3.0

local packages:

@ionic/app-scripts              : 1.3.7
@ionic/cli-plugin-cordova       : 1.3.0
@ionic/cli-plugin-ionic-angular : 1.2.0
Cordova Platforms               : android 6.2.3
Ionic Framework                 : ionic-angular 3.3.0

System:

Node       : v7.9.0
OS         : Linux 3.13
Xcode      : not installed
ios-deploy : not installed
ios-sim    : not installed
@snirs90
Copy link
Contributor Author

snirs90 commented Jun 4, 2017

Also I've saw that no matter on which ion-element (at least few ion elements I've needed) I put no-padding attribute - it got override by some html[dir="rtl"] ... css class.
for example it happens for ion-item also.

@AmitMY
Copy link
Contributor

AmitMY commented Jun 4, 2017

That is intended* behavior.

Reason: Step 1 does not effect anything for ltr users, step 2 changes the entire css bundle.

There are many more RTL fixes coming soonish.

*intended - we don't want it, but the first rtl fixes were intended to override the ltr design

@jgw96 jgw96 added rtl labels Jun 5, 2017
brandyscarney pushed a commit that referenced this issue Jun 6, 2017
…#11635)

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

* style(item): fix spacing

* fix(item): add backwards support for left/right in ng-content

* fix(item): deprecated old variables, not breaking change

* feat(rtl): padding mixin

* feat(rtl): change all padding variables to start/end
add support for old variable names

* feat(rtl): replace all padding-side with start/end

* revert(functions): remove mixins

* feat(scss): add padding-horizontal and rtl functions (thanks brandy)

* feat(padding): use padding horizontal mixin everywhere

* feat(padding): use padding horizontal mixin everywhere

* fix(lint): change properties order. tests passing

* fix(sass-functions): reorder functions to avoid warning

* fix(scss): fix variable name

* perf(rtl): add check if need rtl selector

* feat(scss): add full padding function

* feat(scss): add border-radius mixin

* fix(rtl): change border-radius to use mixin

* perf(scss): only override if has something to override

* feat(scss): add margin scss variables for sides

* feat(scss): add margin mixin

* fix(scss): fix wrong support for 2/3 args

* feat(rtl): spread margins/paddings

* feat(rtl): spread margins/paddings

* feat(position): add rtl support for absolute

* fix(rtl): add missing calls

* fix(item): old attributes deprecated support

* revert(changelog): not intended to be changed

* fix(sass-functions): and not &&

* fix(padding): merge + missing padding

* style(): remove newline

* refactor(mixins): move mixins to mixins file

* style(): fix alignment

* fix(item): right padding should not be set

* fix(): incorrect defaults

* feat(scss-lint): disable some side variables

* fix(scss): lint passes

* feat(lint): disabled text-align

* fix(): correct variable name

* fix(fab): missed a comma

* fix(rtl): rtl method incorrect for multiple selectors

* fix(rtl): toolbar bad merge

* fix(rtl): icon-only is in px not em

* fix(rtl): toggle padding

* feat(rtl): correct notation for rtl custom

* Merge branch 'breaking-item' into start-end

# Conflicts:
#	src/components/checkbox/checkbox.ios.scss
#	src/components/checkbox/checkbox.md.scss
#	src/components/checkbox/checkbox.wp.scss
#	src/components/item/item.ios.scss
#	src/components/item/item.md.scss
#	src/components/item/item.wp.scss
#	src/components/radio/radio.ios.scss
#	src/components/radio/radio.md.scss
#	src/components/radio/radio.wp.scss
#	src/components/toggle/toggle.ios.scss
#	src/components/toggle/toggle.md.scss
#	src/components/toggle/toggle.wp.scss

* feat(rtl): optimize bundle result

* feat(float): use new standard

* feat(platform): gotta have direction on html

* fix(scss): add import + change code order

* fix(lint): passes

* fix(scss): fix for deprecated usages

* fix(scss): property use #{}

* fix(rtl): change css specificity, increase bundle size for multidirectional

* fix(scss): mixin manages priority

* fix(scss): select icon

* fix(scss): correct range variable

* fix(lint): unused import

* fix(multi-dir): things that ignore PropertySpelling must be under multi-dir

* fix(multi-dir): things that ignore PropertySpelling must be under multi-dir

* fix(include-rtl): change to app-direction, to be able to set rtl only apps

* fix(rtl): remove incorrect split-pane rtl behavior

* fix(menu): start needs position on rtl

* fix(float): renamed test file, add float mixin

* fix(scss-lint): lint excluded files

* fix(scss): add missing unit

* refactor(scss): change dir to direction, as the correct property name

* fix(app-direction): replace last usage of include-rtl

Closes: #11805 #11914
@brandyscarney
Copy link
Member

Fixed by #11635, thanks @AmitMY 😄

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 2, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants