search

ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.09k stars 13.51k forks source link

New LTR/RTL styles on 3.4.0 override custom CSS rules #12053

Closed FdezRomero closed 7 years ago

FdezRomero commented 7 years ago

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: The new CSS rules introduced in 3.4.0 for LTR/RTL are prefixed with the attribute selector [dir="ltr"] or [dir="rtl"] (from the root <html> tag), but these rules always having a higher specificity than the ones you would normally write inside a page break the custom styles of our app.

The only solutions we came up with are adding specificity to our current selectors (would require to top 4 classes/attributes), using !important on every conflicting rule (not recommended for so many reasons) or prefixing the page selectors in every SCSS file, like [dir] page-home {...}.

Expected behavior: LTR/RTL selectors to use a lower specificity so we can override Ionic rules easily and cleanly, as we have been doing. Splitting problematic LTR/RTL joint rules ([dir="ltr"] .some-class, [dir="rtl"] .some-class) in two would halve specificity to 0, 0, 2, 0.

Steps to reproduce:

  1. Add a <button ion-button class="my-button"></button> to a page.
  2. Try to style the button from SCSS, like page-name { .my-button { padding: 16px; margin: 0; } }.
  3. Your rules are overwritten by the [dir="ltr"] .button-(mode), [dir="rtl"] .button-(mode) selector.
  4. This happens with almost every component: ion-card, ion-card-content, ion-chip, etc.

Related code: N/A

Other information: N/A

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

global packages:

    @ionic/cli-utils : 1.4.0
    Cordova CLI      : 7.0.1
    Ionic CLI        : 3.4.0

local packages:

    @ionic/app-scripts              : 1.3.7
    @ionic/cli-plugin-cordova       : 1.4.0
    @ionic/cli-plugin-ionic-angular : 1.3.1
    Cordova Platforms               : android 6.2.3 ios 4.4.0
    Ionic Framework                 : ionic-angular 3.4.0

System:

    Node       : v6.10.3
    OS         : macOS Sierra
    Xcode      : Xcode 8.3.3 Build version 8E3004b
    ios-deploy : 1.9.1
    ios-sim    : 5.1.0
    npm        : 5.0.3
brandyscarney commented 7 years ago

@FdezRomero Thanks for the issue! Are you only using one direction or does your app require both? If you only need one direction can you set the following variable in your Sass and let me know if that works for you:

$app-direction: ltr;
AmitMY commented 7 years ago

That is fair. What I suggest doing is either setting a strict direction like @brandyscarney suggested (which does not use [dir=ltr]), or use our new sass mixins to handle multi directional apps. See our newly released blog post in the matter.

FdezRomero commented 7 years ago

Thanks @brandyscarney @AmitMY! I confirm that adding $app-direction: ltr; in variables.scss removes the LTR/RTL selectors and everything works as usual. 🎉

It would be a good idea to add this to both the changelog and release info, as it may be considered a breaking change.

brandyscarney commented 7 years ago

@FdezRomero I am releasing v3.4.1 which will set it to ltr by default. It should be out any minute.

FdezRomero commented 7 years ago

Thanks for the heads up @brandyscarney! You are doing an amazing job, you really are 😎👌

brandyscarney commented 7 years ago

Thank you! So I actually just released a 3.4.2 after finding an issue with changing the default $app-direction - so it will be ltr by default, can be set to rtl for rtl only or multi for both. We'll have the docs updated to reflect this soon. Thanks for bringing this to our attention!

wethinkagile commented 7 years ago

This has cost us 2 weeks of debugging time.

AmitMY commented 7 years ago

@nottinhill Next time:

Setting that "cost" in $ is really irrelevant, as there are many things you could have done to solve this in 3-8 hours, tops, depending on timezone.

^^I read this message just now, and it sounds a bit aggressive. That is not my intent :), I just want to inform you of what to do next times when something as major as this happens.

supryin commented 7 years ago

@AmitMY would you kindly point me to a section of the changelog, where it mentions that custom css gets overwritten and the fix of that bug? In a complex development environment it is not trivial to understand that this is the issue. You are developing a big and popular framework. If your major release has a major bug like this, which is really easy to find during the testing of your release, as a developer I would expect a big notice about that bug (like "BEWARE OGRES") and a sorry from your side.

AmitMY commented 7 years ago

I can't point you in 3.4.0, as there was no announcement of a breaking change. But, it is just step 1. Steps 2 and 3 are still valid for this case, and each could have got your issue fixed immediately.

This was a non-desired breaking change, and we ran the tests inside the app multiple times, and there was a nightly released that was requested for maintainers to see if it breaks their app, and no one said anything, it was assumed that the version is solid. However, If you do read the CHANGELOG, 1 day after 3.4.0 there was a patch released 3.4.1 which fixed this.

I am the one who created this breaking change, and really, didn't think through all of the cases, as v3.4.0 includes a massive amount of changes to the styling (at least 50% of the css selectors in this project), and the tests normally cover style issues, A nice read about the tests we ran.

I don't see a need to apologize. You have a bug on an open source project? It is your duty (as a user of an open source project) to create an issue, like this issue creator did. Plus, you can always downgrade to a previous version and life would be dandy. (even if you don't know what causes the issue: "v3.4.0 styles are not the same as v3.3.0" is all you have to say for people to get what is the problem or at least investigate.

Note that I do not speak for the employees of Ionic, I am just a guy giving my free time to perfect this awesome framework.

wethinkagile commented 7 years ago

Thanks for your thoughts. However I must say, since we started using Ionic 2 Beta we burned through a lot of resources and money, nerves, tears and sweats. I am in the business of building reliable infrastructure which seems in the case of ngx and ionic not to be possible without a dedicated DevOps Team RSS-ing onto Ionic's changelog stream 24/7. We never experienced anything as buggy and ever-changing like Ionic and I am really looking forward to future projects ∈ \ Ionic.

AmitMY commented 7 years ago

I am a one man team, and I manage to get a grip on the changelog on all of the packages I star on github using sibbell. I do wanna correct you on one thing, there is no "changelog stream", it is more like a per-release change, so more like a blog thing.

Glad to help clarify everything. It is your choice what to use, and thanks for using Ionic at least for now.

mhartington commented 7 years ago

Locking this issue as the conversation here is not productive. If there are any issues, please be sure to update the the latest release. 3.4.0 had a unintentional breaking change, we addressed it by releasing 3.4.2 and notifying people of the release and what it fixes in the changelog.