Igniteui-angular: Return normalize() for backwards compatibility

Created on 4 Mar 2019  路  7Comments  路  Source: IgniteUI/igniteui-angular

Is your feature request related to a problem? Please describe.

Upgrading my application to 7.2.0-rc.1 causes the layout of the application to change drastically. I understand this is caused by the normalization removal, but I think that css normalization should remain in the theming and be controllable by the developer.

Describe the solution you'd like

  1. Restore /core/styles/base/_normalize.scss and remove list normalization from it (leave list normalization to be controlled by component themes). Rename it to igx-normalize.
  2. Don't call igx-normalize() in the theme mixins. Leave it be controlled by the developer.
  3. Create an update migration that adds the following to the styles.scss:
// Comment out to restore the browser CSS defaults, or if other normalization is used.
@include igx-normalize()
  1. Update igniteui-cli projects to @include igx-normalize()
  2. Update ng add migration to @include igx-normalize()
  3. Update theming documentation to @include igx-normalize() and with a paragraph on what it does and when to comment it out.

Describe alternatives you've considered

I copied the contents of the removed _normalize.scss and removed the portions with list style resets and included it in the styles.scss to restore the application to the state before upgrading. This however is still a breaking change.

@import "~igniteui-angular/lib/core/styles/themes/index";
@import "normalize";

@include normalize();
@include igx-core();

Additional context

Already existing applications should not be affected drastically by changes, regardless of how they were requested. The original default should be preserved, but users that are affected by the original state of the code/style should be provided with options to not use them, instead of completely removing the code/style.

feature-request material-theme resolved blocking 7.2.x

Most helpful comment

@kdinev @simeonoff
I've tested with both packages - sass package seems to be a version behind but the end result look the same to me. However, something I've noticed with both - they don't touch box-sizing for the most part. That is still in our theme:
Use this one, it's the closest to what ours used to be. We also need to make sure this PR gets merged.

Also there's a new paragraph margin reset, that at least in the Ignite UI CLI of where we have the typography class on the body, overrides the default top+bottom margin with double top and 0 bottom.
This bit I think produces .igx-typography p, .igx-typography .igx-typography__body-1 rule

@desig9stein can you add bottom margin of 16px to the default typography scale for the body-1 rule?

All 7 comments

@kdinev I think this puts a strain on us by requiring us to support another module, such as browser resets/normalization.

You are on point when saying it's the developer's responsibility to "normalize/reset" the native elements across browsers, such that your application looks and behaves the same. We, however, should only be responsible for our components only, ensuring they look and behave the same in all browsers.

After the update to 7.2.0-rc.1, our components should continue to work and behave the same. What you are probably experiencing is some layout changes in your application.

I would recommend you switch to an existing scss normalization package.

@simeonoff That makes sense. For backwards compatibility, let's add a dependency on this package for users going through ng update, ng add or the igniteui-cli. We can make it opt-in, the same way as described in the issue. Include it for them, and they can comment it out. @damyanpetev @bazal4o

One correction:

  1. ng add should be opt-in with a prompt.
  2. ig new should be opt-out, as the project templates already depend on normalization.
  3. ng update should be opt-out, as existing projects depending on normalization should not be broken.

For option one should the prompt default to yes, or no?
Option 2 and 3 will be updated as above.

@bazal4o Default: Yes.

@kdinev @simeonoff
I've tested with both packages - sass package seems to be a version behind but the end result look the same to me. However, something I've noticed with both - they don't touch box-sizing for the most part. That is still in our theme:
https://github.com/IgniteUI/igniteui-angular/blob/2ab13953adea4e6d7dfd900f0ec7ff87a4c1b57f/projects/igniteui-angular/src/lib/core/styles/themes/_core.scss#L61-L65
Intended?

Also there's a new paragraph margin reset, that at least in the Ignite UI CLI of where we have the typography class on the body, overrides the default top+bottom margin with double top and 0 bottom.
This bit I think produces .igx-typography p, .igx-typography .igx-typography__body-1 rule:
https://github.com/IgniteUI/igniteui-angular/blob/2ab13953adea4e6d7dfd900f0ec7ff87a4c1b57f/projects/igniteui-angular/src/lib/core/styles/typography/_typography.scss#L52
Not sure how we like that, but most CLI templates relied on that padding to put some description above a component. Any specific reason for the lack of bottom margin?

@kdinev @simeonoff
I've tested with both packages - sass package seems to be a version behind but the end result look the same to me. However, something I've noticed with both - they don't touch box-sizing for the most part. That is still in our theme:
Use this one, it's the closest to what ours used to be. We also need to make sure this PR gets merged.

Also there's a new paragraph margin reset, that at least in the Ignite UI CLI of where we have the typography class on the body, overrides the default top+bottom margin with double top and 0 bottom.
This bit I think produces .igx-typography p, .igx-typography .igx-typography__body-1 rule

@desig9stein can you add bottom margin of 16px to the default typography scale for the body-1 rule?

I'm OK with the suggestion by @simeonoff . Go for it.

Was this page helpful?
0 / 5 - 0 ratings