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

Stop nested .from-top classes causing two modal top bars #61

Closed
wants to merge 11 commits into from
Closed

Stop nested .from-top classes causing two modal top bars #61

wants to merge 11 commits into from

Conversation

DeanmvSG
Copy link

A package I am using creates a modal and then applies the from-top class to the outer class of the modal. This duplicates that class as and so it causes this to happen:
screen shot 2017-03-20 at 14 21 03
👎

This change means that if and other packages to this it resets the inner from-top to 0px which doesn't cause the visual issue and looks like this:
screen shot 2017-03-20 at 14 22 57
👍

I'm not sure if other packages are doing it but though it seemed easier to fix it here incase they are rather than in each package!

Thanks!

@arcticicestudio arcticicestudio self-requested a review March 26, 2017 16:13
@arcticicestudio
Copy link
Contributor

arcticicestudio commented Mar 26, 2017

Thanks for your contribution 👍
Currently I don't see the benefit of this addition since it has absolut no effect as seen from the perspective of the theme package. There is no class duplicated .from-top selector in any Atom core package which would make use of this.
Also as you can see on the screenshot below the top: 0 gets disabled by Atom anyway since the top: 20vh value overrides all node selectors inside of the top .from-top class.

ghi-61-scrot-debug-modal

@DeanmvSG It would be nice if you can post the name of the package so I can try to reproduce this behavior.

It looks like the specific package should fix the way it creates the modal. The Atom API provides powerful methods to create a package that fits the current theme without setting any custom CSS which may break the UI theme.

I'd like to help you fix this problem, maybe I can reproduce it and create a bugfix PR in the repository of the specific package.

@DeanmvSG
Copy link
Author

I didn't know if it could be happening with other packages as well so not sure if here was the best place to do it?

I've opened an issue (linked above) over at the other package and then tested removing the CSS classes locally and it all still worked ok so I'd say it was best to fix in other package if you'd prefer! 👍

@arcticicestudio
Copy link
Contributor

arcticicestudio commented Mar 27, 2017

I've investigated the problem in debug mode and it seems like there is indeed a strange bug in the theme. It seems like the div rendered by the remote-ftp package is affected by the top: 20vh attribute and gets moved down relative to its origin: the <atom-panel> container which should normally be nested inside.

origin atom-panel container

remote-ftp div container

I am very glad about your help 💚 since your PR would avoid most of the visual issues, but there will still be a small visible overlap between both container and the origin atom-panel container would still get rendered behind the scenes.

Screenshots using the PR branch:
ghi-61-scrot-debug-pr-origin
ghi-61-scrot-debug-pr-div

I'll refactor the whole code for overlays- and modals to fix this strange issue since it won't occur with other themes. This will be implemented in the UI scale refacroring #58.
I'll @-notify you as soon as I've updated the linked issue and psuhed a branch to test the changes 😄


To be resolved in #58

@arcticicestudio arcticicestudio changed the base branch from master to develop January 8, 2018 10:20
@DeanmvSG DeanmvSG closed this Jun 19, 2019
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.

2 participants