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

[audit] overlay extra classes #6372

Closed
willshowell opened this issue Aug 9, 2017 · 10 comments
Closed

[audit] overlay extra classes #6372

willshowell opened this issue Aug 9, 2017 · 10 comments
Assignees
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@willshowell
Copy link
Contributor

willshowell commented Aug 9, 2017

Bug, feature request, or proposal:

Suggesting library-wide audit

Currently

Overlay components all accept extra classes differently:

Ideally

Consistent NgClass compatibility would be awesome. If not possible, string | string[] support would be good.

Additionally, consistent naming would be nice. Snackbar uses extraClasses, whereas select uses panelClass. Also, select and dialog both use panelClass, but the class is added to different elements.

@jelbourn jelbourn added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Aug 16, 2017
@jelbourn
Copy link
Member

Agreed, they should all be consistent. I would want to use panelClass everywhere

@amcdnl
Copy link
Contributor

amcdnl commented Sep 24, 2017

@jelbourn - So to clarify, update all the inputs to panelClass with NgClass as the interface? Do we need to deprecate as well?

@jelbourn
Copy link
Member

The template-based components can have NgClass mechanics, but dialog.snack-bar would just stay as strings because there's no binding context

@jelbourn
Copy link
Member

Also this would be a good candidate for a mixin

@amcdnl
Copy link
Contributor

amcdnl commented Sep 30, 2017

@jelbourn -

  1. I played around with a mixin but the implementations varied so much, it was tricky. I ended up making a utility parser instead.
  2. Do you want me to rename matTooltipClass to matPanelClass or just leave it for that one?

Before I make a PR up, can you give this a once over and see if this is on track with what you were thinking? https://github.com/amcdnl/material2/tree/overlay-class-6372

amcdnl added a commit to amcdnl/material2 that referenced this issue Sep 30, 2017
@jelbourn
Copy link
Member

jelbourn commented Oct 1, 2017

Tooltip can stay as matTooltipClass

@jelbourn
Copy link
Member

jelbourn commented Oct 1, 2017

If you make a PR it's easier to leave comments; you can mark it as WIP

@jelbourn
Copy link
Member

jelbourn commented Oct 1, 2017

One thing I will say is that we don't want to add additional cdk packages for utility methods; each one is a public API that a user would potentially need to know about (such as when adding to a systemjs config)

@josh18
Copy link

josh18 commented Oct 27, 2017

Would it make sense (maybe in another issue / PR) to add a similar input for the .cdk-overlay-container element?

I guess another option would be to have only one class at the top (.cdk-overlay-container) and then devs can use nested styles to control all parts of the overlay. With the current implementation I don't think there is any way to edit an overlay background from a component?

mmalerba pushed a commit that referenced this issue Oct 30, 2017
* fix(overlay): overlay class audits #6372

* chore(nit): pr feedback

* chore(nit): remove param
@amcdnl amcdnl closed this as completed Dec 2, 2017
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

4 participants