Skip to content

Components should create a display:block style by default #25856

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

Closed
SanderElias opened this issue Sep 7, 2018 · 9 comments
Closed

Components should create a display:block style by default #25856

SanderElias opened this issue Sep 7, 2018 · 9 comments
Labels
area: core Issues related to the framework runtime
Milestone

Comments

@SanderElias
Copy link
Contributor

SanderElias commented Sep 7, 2018

I'm submitting a...


[ X ] Feature request
[ X ] Documentation issue or request

Current behaviour

When you create a new component, by default it gets no setting for css display.

Expected behavior

it should have display:block by default.

What is the motivation/use case for changing the behavior?

I know this has been discussed before (#5960), and pops up in (#12340) too.
Still, I'm putitng this back on the roll. For the following reasons:

  1. Current behaviour is confusing for new devs
  2. Devs need display:block most of the time (for me, its 99+%)
  3. The web doesn't break if we put this in place.
  4. The expected impact of this "breaking" change is minimal
  5. Saves a few bits from the app's payload
  6. Less questions. no more, why does my css....
  7. One more thing less to remember when creating a component.

I want to elaborate on point 3 a bit. The same discussion has been held in by standarts committee, If you follow along that discussion you will see that the general consensus is that it is actually a good idea, but it can't be changed because that would break the web. Angular does not have that issue. Angular can change this, and the impact of the change would be minimal. probably something that has less impact as for example rxjs 5 to 6.

Others:
If we don't do this change, at least there should be more emphasis on this in the documentation.

@IgorMinar IgorMinar added the area: core Issues related to the framework runtime label Sep 7, 2018
@ngbot ngbot bot added this to the needsTriage milestone Sep 7, 2018
@Meligy
Copy link
Contributor

Meligy commented Sep 8, 2018

This issue root is in how Angular keeps the component custom elements with their custom tag names in the rendered DOM, unlike other frameworks that omit these tags from output like React.

The design decision might not be bad overall, and is probably here to stay, but a problematic effect of this design is that these component elements get the display: inline; treatment by default. That's what browsers do when they see tags with no style, including built in user agent styles.

It's an instance where lack of action (no display setting) is equal to an action (setting display to inline).

Since it's not even inline-block, this often causes weird results when you try to use block styles to child elements in the template+styles of the component. The margins etc don't work as expected, and the Angular user has to track it all the way to understanding the above behaviour in order to understand and fix the problem.

This is where a documentation addition might help, also maybe using the Angular CLI default component schematic to add display: block; and/or a comment describing the behaviour, to default component styles.

@SanderElias
Copy link
Contributor Author

SanderElias commented Sep 8, 2018

@Meligy I'm aware of the history of this issue. I agree with the design choices. Still, Angular is in a position where we can change this browser default. The browsers can't do this, but Angular can.
Browsers behaviour as a default sounds good. However, in practice this thing more of a nuisance, as a benefit.
As in practice, you have a particular thing that is mandatory 99% of the time, and optional in 1%, it's a good idea to flip this around. No matter what, or how sound, the originating design choice is.

Angular is in a unique position, that can make the display:inline the option, and display:block the default.
Angular has also directives, aside from components. Those are here for the use-cases where you want a display:inline (or no real representation in the DOM at all.)

The cognitive load of Angular is already pretty high. Changing this would lower it a bit.

@trotyl
Copy link
Contributor

trotyl commented Sep 8, 2018

@SanderElias I don't think this is about component, but only for custom element. A directive could also have element selector, like <mat-tree-node>, people might want that to be block as well. But a component may not use custom element, like <button mat-button>, adding a display: block; for <button> doesn't make any sense.

@chaosmonster
Copy link

I agree. I do this so many times that it feels a little bit cumbersome.
What I tried some time ago and it works under certain circumstances is to add a global css style

app-* {
  display: block;
}

That way I can override it where ever I need to.

Maybe something like this (using the prefixes defined in the angular.json) would be an option.
And I am aware that this wouldn't work with ViewEncapsulation.Native. It's more meant as a input to our creative thinking process.

@robwormald
Copy link
Contributor

Thoughts: I agree with the stated problem (I forget this myself all the time!) but I'm not convinced that changing the default is a good idea. RE: cognitive overhead - my argument would be that any time that Angular diverges from the default browser behavior, that in itself increases the cognitive overhead (and load on docs team, etc).

It would likely break a number of apps as well - at least if we were to change something in the internals of Angular/Renderer, and (sorry) I'd argue again there's an increase in cognitive load here, because now something "invisible" (not in your component.css file) is changing behavior (and now you have to go look at the docs! How do I override it? etc )

If we want to avoid invisible / non-local problems (that is, I generally shouldn't have to look elsewhere to understand how a component works), I think that means that defaults like this have to exist in your app-component.css file.

We could use schematics or similar to augment the default css template, so ng g c foo would create a foo.component.css file like this out of the box:

:host {
  display: block;
}

One nice thing here is that many new developers have no idea that the :host selector is even a thing, so generating one for them teaches them something new for free.

One drawback here is that you then have to keep things "in sync" - if you change those default later, they won't affect already generated components.

We could provide a app/defaults.css file, that is added to the styleUrls of a new component:

:host {
  display: block;
}
@Component({
  styleUrls: [
   'app/defaults.css',
   './foo.component.css'
  ]
})

There's an argument to be made for CSS Variables (Custom Properties) here as well, because in a modern browser, rather than duplicating the app.defaults.css file per component, you would load it in index.html and do:

:root {
  --default-component-display: block;
}

Then the generated foo.component.css would look like this:

:host {
  display: var(--default-component-display);
}

This one feels the best to me:

  • it's explicit
  • sticks close to the platform (so you can go read MDN about CSS Variables, rather than Angular documenting a slightly different custom API.

This requires CSS Custom Property support - the good news is this is every browser BUT IE nowadays: https://caniuse.com/#feat=css-variables

Since Angular does require a compiler pass, we could potentially make a best effort to "downlevel" those properties - at compile time, rewrite the dynamic var(--some-variable) to with the variables specified in the app.defaults.css file. This would not allow for dynamically setting properties at runtime, but would provide a middle-ground until we drop IE support.

CSS is forgiving, so if we were to generate something like this - on IE, the 2nd line doesn't parse (and so the 1st line is used) - on browsers that support CSS Variables, the --default-component-display is used (or the fallback block if not defined)

:host {
  display: block;
  display: var(--default-component-display, block);
}

This is broadly useful for theming as well, and potentially Material.

@SanderElias
Copy link
Contributor Author

@robwormald I would even prefer this over changing the default. I don't agree that changing the default would raise the cognitive load. But let us not go there, as we agree on the solution path.

I prefer your solution because as you said, it exposes new devs to :host :{}. This does even a better job in discouraging them to not use a wrapping element in their components.
And in the same go, it shows them about the display:block, which most of them not even realize is a thing and might encourage them to learn some stuff about CSS.

Also exposes it them to CSS custom properties. which is another plus. It is about time that this will get some more exposure and broader pick-up.

I could live with the fall-back for IE, but I would like to see a setting to turn that off. Ie support should be a thing of the past. It is for me already (for a long time), but aside from that, I don't think we should carry this forward. Having a setting means that in a couple of years, we can switch the default of the setting, to not generate the IE fallback anymore(but still be available for devs that still need to support Ie).

@chaosmonster
Copy link

I really like the idea to solve this via schematics.

About resolving css variables at build time.
In a way I like the idea, but I would like to point out as @SanderElias said, that this should be a compile flag.
What I could imagine, and this should be a separate issue, is a preprocessor step within webpack, that resolves all css variables. This seems so generic, that I think it might already exist or at least should be out of scope of the angular compiler or what so ever.
Alternatively to resolving the variables it could also add css lines as shown in @robwormald last example.

@hansl
Copy link
Contributor

hansl commented Sep 11, 2018

This issue was moved to angular/angular-cli#12244

@hansl hansl closed this as completed Sep 11, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime
Projects
None yet
Development

No branches or pull requests

7 participants