Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(modal): Add CSS class(es) to the top modal window #2524

Closed
wants to merge 1 commit into from
Closed

feat(modal): Add CSS class(es) to the top modal window #2524

wants to merge 1 commit into from

Conversation

R3VoLuT1OneR
Copy link

Adds ability to add CSS class(es) to the top modal window.

Can be used for hiding other modal windows with CSS if there are multiple modal windows opened.

@pkozlowski-opensource
Copy link
Member

@R3VoLuT1OneR by "top window" you mean the one on top of the stack of windows if multiple modals are opened, right? If so it should be working correctly - I mean - modals that are not on top of the stack should be hidden below the top-level one. If this is not the case that would be a bug to be fixed.

I'm not big fan of yet another option to introduce class name. What we could do instead is to have a boolean model value exposed to the window template so people could override this template and add whatever classes they need.

At present I'm not a big fan of enlarging the API surface of the $modal service, it is kind of too complex already. But I would like to hear more about your exact use-case before closing this PR.

@R3VoLuT1OneR
Copy link
Author

Yes, "top window" it is one on the top of the stack of popup windows, all other windows not hidden and I didn't find any documentation, code or tests that mentioned this. The problem that I tried to solve with adding class to top modal, it is hiding all other modal windows. You can open bug (issue) and assign it on me, I will fix it.

But I am afraid that this bug fixing gonna affect behavior of modal windows service. So I think we still need to enlarge modal API with boolean option that would represent ability of hiding not top modal windows, false by default so it is will not affect previous behavior.

@wesleycho
Copy link
Contributor

Can you explain the difference between this and the existing windowClass feature?

@R3VoLuT1OneR
Copy link
Author

@wesleycho the difference is that windowTopClass is applied only on the top window ( the first one in the stack of windows ) and windowClass is applied for all windows. This class is useful only if multiple popup windows are opened.

One more time. The problem I tried to solve, is the hiding of bottom popup windows, if multiple windows are opened.

@wesleycho
Copy link
Contributor

I don't quite understand - we do not support multiple modal windows being opened at the same time. Is what you want a feature for supporting multiple modal windows being opened at the same time?

@chrisirhc
Copy link
Contributor

We do support multiple modals through a modal stack. We can consider this but I think we should put this off to merge after the 0.13 release.

@wesleycho
Copy link
Contributor

Hm, I'm completely mistaken then, whoops.

@wesleycho wesleycho modified the milestones: 0.13.x, Purgatory Apr 5, 2015
@karianna
Copy link
Contributor

karianna commented Apr 7, 2015

@R3VoLuT1OneR Can you rebase with master please?

@R3VoLuT1OneR
Copy link
Author

@karianna rebase done, sorry for slow response

@wesleycho
Copy link
Contributor

Question - could the windowTemplateUrl option be used to replace the template to currently accomplish what is desired?

@wesleycho
Copy link
Contributor

I misunderstood this feature before - I see what this is doing now.

If the class is only to be applied to the top modal, shouldn't the class be removed from the other modals?

Thinking about this, I am not sure we would want this - what are your thoughts @R3VoLuT1OneR?

@R3VoLuT1OneR
Copy link
Author

@wesleycho I am not sure if I understand you correctly.

windowTopClass - Applied only on the top modal window ( first one in the stack of modal windows ) on other modal windows should not be that class.

Usage: Show only top modal window, and hide others with css. ( .windowClass {display: none;} .windowTopClass {display: block;} for example).

By default when you opening multiple modal windows they all displayed and it is hard to hide unwanted windows with CSS, only with JS.

@wesleycho
Copy link
Contributor

The problem is when a new modal is opened, we would have to go through and remove this class for all of the other modals then. I am just concerned with potential complexity here, but I guess I can accept this use case.

This PR needs to be rebased though, and after rebasing, the commits need to be squashed into one.

@R3VoLuT1OneR
Copy link
Author

Rebased and squashed.

About removing the "windowTopClass" from all other windows, actually we should remove class only from 1 window ( previous top ) on opening new one and on closing window we should add class to new top window, that what my code doing. There are also tests for this use case.

@wesleycho
Copy link
Contributor

Hm, makes sense - this LGTM.

@wesleycho wesleycho closed this in bd38e8f Sep 10, 2015
jasonaden pushed a commit to deskfed/bootstrap that referenced this pull request Jan 8, 2016
- Added ability to add a class to the most recently opened modal window.
  Note that even if different classes are specified, the class will
only be present if the modal is the most recently opened modal, i.e. if
modal1 was opened with a top class of `foo`, and modal2 is opened
afterwards with a top class of `bar`, modal2 will have the class `bar`
for the modal window, and modal1 will not have the class `foo`.

Closes angular-ui#2524
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants