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): add ability to set custom class on panes #1584

Closed
wants to merge 2 commits into from

Conversation

fxck
Copy link
Contributor

@fxck fxck commented Oct 24, 2016

I re-did #1438

It includes tests for overlay(I didn't see any similar tests for say backdrop class on menu or dialog, so I assumed it's not required since it should inherently work when overlay tests passes) and comments from the previous PR with the exception of #1438 (comment) and #1438 (comment) which didn't make sense to me.

@jelbourn

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 24, 2016
@jelbourn
Copy link
Member

@fxck Would your specific problem be solved by simply having a higher z-index on the overlay container? (right now it's just 1 to create a new stacking context).

@jelbourn jelbourn self-assigned this Oct 25, 2016
@fxck
Copy link
Contributor Author

fxck commented Oct 25, 2016

No, unless I were in control of that z-index. But what might be worth doing is sticking that custom class on container rather than pane. I'm not sure what was the original reasoning for putting it on panes, it's been a while.

I guess having the class on the container rather than on pane would make sense. You wouldn't have to use /deep/ to actually target the pane.. I think anyway.

So how about this:

  1. I'll move the custom class to the container rather than the pane

  2. I'll change the default z-index of container to 1000, because of this http://jsbin.com/mesujat/1/edit?html,css,output (z-index 1000 on pane has absolutely no effect because the container has z-index 1)

  3. I'll change the default z-index of pane and backdrop to 2(pane) and 1(backdrop), and I'll hardcode it, because of the above point, having this configurable doesn't have any sense, since the only behaviour you need inside the container is that pane is above backdrop

@jelbourn

@fxck
Copy link
Contributor Author

fxck commented Oct 27, 2016

I see, the reason why I added it to pane instead of container was that there is only one container.

Then there's this ..

image

..when you open, menu from inside a dialog, the stacking order is not correct, dialog itself it above menu's backdrop.

I think is issue is a bit deeper, there should be some sort of stacking order manager.

@jelbourn @kara

@crisbeto
Copy link
Member

Are there any plans to continue with this PR? Something similar (being able to add a class to the .overlay-pane) should help with an issue where the tooltip can sometimes become hidden immediately.

@fxck
Copy link
Contributor Author

fxck commented Nov 25, 2016

I never got the reply about stacking manager. I think adding classes to either pane or overlay would be only a temporary fix and more issues will come up sooner or later. But I don't want to do anything without having a discussion.

@jelbourn
Copy link
Member

This has been on the backburner since I've been wanting to spend some more time thinking about how stacking should work in general.

@fxck
Copy link
Contributor Author

fxck commented Nov 30, 2016

Stacking aside, this still could be useful, currently there's no way to properly style md-menu(stuff like width).

@fxck
Copy link
Contributor Author

fxck commented Nov 30, 2016

I take it back, there is, it has to be global though.

@mmalerba
Copy link
Contributor

@jelbourn @fxck what's the status of this should it be rebased? closed?

@fxck
Copy link
Contributor Author

fxck commented Mar 14, 2017

for what it was, this PR is obsolete, altho most of my points stands, there still are cases when inability to control z-index of panes and overlays still can cause problems, as well as I think there needs to be a class that handles z-index of various elements.

@mmalerba
Copy link
Contributor

@fxck ok will close this PR then, if you have concerns about z-index that aren't already being tracked by an issue, feel free to open a new issue to discuss them.

@mmalerba mmalerba closed this Mar 17, 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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants