northem / northem-light-atom-ui

A north-bluish, light clean and elegant minimal Atom UI.
MIT License
1 stars 1 forks source link

Fix tab bar placeholder too tall #13

Closed edwardloveall closed 7 years ago

edwardloveall commented 7 years ago

When rearranging tabs, the placeholder was set to be about 40px tall. This would cause the overall tab bar height to increase. That felt pretty weird when you're trying to move a tab and the tab bar is resizing on you.

arcticicestudio commented 7 years ago

Thanks for your contribution 👍 This behavior has also been reported in nord-atom-ui #56 and I was only able to reproduce it on Windows 7 (in a VirtualBox), on Arch Linux (GNOME DE) everything works fine. I'll test it for macOS tomorrow to see if it'll be necessary to wrap your code into a OS-dependent selector to prevent a possible breakage on Linux systems.

Anyway, the build fails because I still have to update to the latest Atom theme package API introduced in version 1.13.0. I hope I can get it done this weekend 😄

edwardloveall commented 7 years ago

Sounds good 😄 I couldn't figure out if there's a base theme that themes inherit from, but it seems like the problem may be coming from that. It's been a while since I've looked into all the intricacies of atom theming

arcticicestudio commented 7 years ago

I've tested your changes on macOS, Windows 7/10 and on my main Arch Linux system. The change does not impact the usage on Linux systems, but if we use a auto calculated height

.placeholder {
  height: auto;
}

we can achieve the exact same behavior on all OS.

Linux

macOS

This should work for the moment, but I've also planned to refactor the whole UI sizing (no more hardcoded px values e.g.) to avoid problems like this. (nord-atom-ui #50, nord-atom-ui #55)

@edwardloveall it would be nice if you can test the snippet above to confirm that it works 😄

edwardloveall commented 7 years ago

height: auto makes the placeholder disappear like in your macOS capture above. Was that the goal? If so, why not just display: none?

Here's a capture of the placeholder not hidden: placeholder

edwardloveall commented 7 years ago

Not sure why the capture turned out grayscale. The placeholder is blue in atom for me.

arcticicestudio commented 7 years ago

It's more a kind of workaround until the refactoring of the UI sizing which should allow to adjust the height to the current height of the tabs.

I also think without the placeholder it looks more flat and smoother. On the other side it could also confuse users when there is no more visual line 😕

I'll rerun your PR build as soon as #19 is merged and will merge this PR 😄

edwardloveall commented 7 years ago

Sounds great, thanks! The visual line isn't essential (for me) so it's not a huge deal either way. Any of these fixes would be great :)

arcticicestudio commented 7 years ago

Alright, no more 🚫 blocked by #19 and the build has been run successfully 😄 The fix will be shipped with the next release version.

arcticicestudio commented 7 years ago

🚢 Shipped in apm package release version 🏷 2.1.0