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

feat(overlay): replace OverlayContainer themeClass w/ addClass/removeClass methods #6975

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

jelbourn
Copy link
Member

@jelbourn jelbourn commented Sep 10, 2017

BREAKING CHANGE: Now that the Overlay is part of the cdk rather than Angular Material directly,
the themeClass property has been removed. To add a class to the
overlay for theming, you can do

overlayContainer.getContainerElement().classList.add('my-theme-class');

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 10, 2017
if (value) {
this._containerElement.classList.add(value);
}
addClass(cssClass: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the getContainerElement() is public, I'm not sure that these methods are even necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I considered that. I was worried that the change would be "too complicated" for people, but maybe I'm overthinking it. I'll just remove the themeClass completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we still need some easy way for people to pass the theme to overlays, though. Perhaps a provider that injects the overlay container and applies the class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is getContainerElement().classList.add('dark-theme') that must worse than themeClasss = 'dark-theme';?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is, but I guess that it's not as obvious that that's the way to theme all components.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO while getContainerElement().classList.add('dark-theme') isn't immediately obvious, it reduces the magic, which is nice.

Just don't forget the example in the theming guide needs to be updated with whichever solution!

@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 15, 2017
@jelbourn jelbourn force-pushed the overlay-container-class branch 2 times, most recently from 5ad9bf9 to 593aa7b Compare September 15, 2017 19:52
@jelbourn
Copy link
Member Author

I went ahead and just removed themeClass. I'm still open open adding a more "obvious" way to apply a theme class, but I think this is good as a starting point.

@@ -169,7 +169,7 @@ Since certain components (e.g. menu, select, dialog, etc.) are inside of a globa
an additional step is required for those components to be affected by the theme's css class selector
(`.unicorn-dark-theme` in the example above).

To do this, you can specify a `themeClass` on the global overlay container. For the example above,
To do this, you can add the approviate class to the global overlay container. For the example above,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "appropriate"

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after correcting the typo.

…Class methods

BREAKING CHANGE: Now that the Overlay is part of the cdk rather than Angular Material directly,
the `themeClass` property has been removed. To add a class to the
overlay for theming, you can do
```ts
overlayContainer.getContainerElement().classList.add('my-theme-class');
```
@jelbourn
Copy link
Member Author

Fixed typo and demo-app (which I missed before)

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Sep 15, 2017
@jelbourn jelbourn merged commit a944f6e into angular:master Sep 20, 2017
@jelbourn jelbourn deleted the overlay-container-class branch April 2, 2018 22:30
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants