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

Fix tab bar placeholder too tall #13

Merged

Conversation

edwardloveall
Copy link
Contributor

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.

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 arcticicestudio self-assigned this Mar 3, 2017
@arcticicestudio
Copy link
Collaborator

arcticicestudio commented Mar 3, 2017

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 😄

  • Reproducible on macOS?
    ➡️ ✔️

@edwardloveall
Copy link
Contributor Author

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
Copy link
Collaborator

arcticicestudio commented Mar 4, 2017

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@arcticicestudio
Copy link
Collaborator

arcticicestudio commented Mar 4, 2017

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
Copy link
Contributor Author

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
Copy link
Collaborator

arcticicestudio commented Mar 4, 2017

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

@arcticicestudio arcticicestudio merged commit fad4812 into northem:develop Mar 4, 2017
@arcticicestudio
Copy link
Collaborator

arcticicestudio commented Mar 5, 2017

🚢 Shipped in apm package release version 🏷 2.1.0

@arcticicestudio arcticicestudio added this to the 2.1.0 milestone Jul 16, 2017
@arcticicestudio arcticicestudio removed their assignment Sep 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants