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

BUGFIX: Show delete asset dialog in edit asset media view #1158

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

aertmann
Copy link
Contributor

@aertmann aertmann commented Oct 4, 2016

When clicking the delete asset button in the footer of the edit asset view in the media browser/module the dialog isn't shown due to overflow on the sticky footer is hidden. To circumvent this the dialog html is placed outside the footer.

When clicking the delete asset button in the footer of the edit asset view in the media browser/module the dialog isn't shown due to overflow on the sticky footer is hidden. To circumvent this the dialog html is placed outside the footer.
@mention-bot
Copy link

@aertmann, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hhoechtl to be a potential reviewer.

@robertlemke
Copy link
Member

@aertmann I can't really reproduce this - I tried it with different resolutions, maybe something I'm doing wrong?

Here's how it looks with current 2.1 without your patch applied:

image

@gerhard-boden
Copy link
Contributor

gerhard-boden commented Oct 4, 2016

Hi @aertmann! Can't reproduce it either and tested this dialog before (and see it almost on a daily basis on live systems). Maybe I'm doing something different.

Tried it on 2.1 and on master, no luck

@aertmann
Copy link
Contributor Author

aertmann commented Oct 4, 2016

@robertlemke @gerhard-boden: just checked again, seems to only be a problem in Chrome, sorry assumed it was cross browser as that would make perfect sense.. maybe in recent update something changed.. did you guys try Chrome or another browser?

@gerhard-boden
Copy link
Contributor

gerhard-boden commented Oct 5, 2016

I was in Chrome, no problems there. (Win10)
Maybe we can compare version number (not at the computer at this moment).

UPDATE: Chrome 53.0.2785.143
Tested on Neos 2.1: The dialog appears. Read up on the actual issue. According to my research the element with position fixed should not care if the parent element has overflow: hidden.

@robertlemke
Copy link
Member

I tried it with Chrom (Mac) Version 53.0.2785.116 (64-bit). Also works with the patch applied - so if that solves a problem for you, @aertmann, let's merge it.

@aertmann
Copy link
Contributor Author

aertmann commented Oct 5, 2016

hmm that's pretty odd, can reproduce it on neos.io (https://www.neos.io/neos/management/media/edit?moduleArguments%5Basset%5D%5B__identity%5D=4891ec44-4154-4429-bfd4-292fee161d5e) and using Chrome 53.0.2785.116 (64-bit) on macOS as well (even in incognito)

@aertmann
Copy link
Contributor Author

aertmann commented Oct 5, 2016

hmm the neos-footer container needs to have the fixedsticky-on class and not fixedsticky-off for it not to work.. it changes if scrolling to the bottom of the page

@aertmann
Copy link
Contributor Author

aertmann commented Oct 5, 2016

else try making your browser window a little smaller to reproduce it

@robertlemke
Copy link
Member

robertlemke commented Oct 5, 2016

Okay, I can reproduce it with a very small browser window - the trick is that you must not scroll to the bottom but stay on the top.

image

The above screenshot shows that the dialog is obstructed.

@robertlemke robertlemke merged commit 501f65a into neos:2.1 Oct 5, 2016
@gerhard-boden
Copy link
Contributor

Tried around a bit but still couldn't reproduce it. neos-footer has overflow :hidden no matter if the fixedsticky-on is set or not. What it does is changing from position: static to position: fixed. Maybe it has to do with z-index? Well... nevermind now, just wanted to figure it out, out of curiosity.

But the good news it's fixed now 😄 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants