nordtheme / atom-ui

An arctic, north-bluish clean and elegant minimal Atom UI theme.
https://www.nordtheme.com/ports/atom-ui
MIT License
95 stars 15 forks source link

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

Closed DeanmvSG closed 5 years ago

DeanmvSG commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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