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

Reduce nav tab border width and adjust sidebar padding for improved layout #2040

Merged
merged 10 commits into from
Jan 21, 2025

Conversation

hichemfantar
Copy link
Contributor

@hichemfantar hichemfantar commented Jan 20, 2025

Decrease the navigation tab border width and remove unnecessary padding in the sidebar for a cleaner layout and more horizontal space.

before
image

after
image

Copy link

github-actions bot commented Jan 20, 2025

@hichemfantar hichemfantar marked this pull request as ready for review January 20, 2025 14:31
@josevalim
Copy link
Member

@hichemfantar let's go ahead and reduce the left border of the active selection as well for consistency? I believe those two are meant to be in sync. :)

@hichemfantar
Copy link
Contributor Author

all good

@josevalim
Copy link
Member

The left entries are no longer aligned:

Screenshot 2025-01-20 at 18 13 10

We need to either bring everything a bit more to the left or the "Pages" a bit more to the right. Sorry for the pixel shaving but it feels this is the type of feedback you would want to give/receive. :)

@josevalim
Copy link
Member

josevalim commented Jan 20, 2025

Also, for a future somewhat related discussion, @DavidOliver @jonatanklosko @liamcmitchell @wojtekmach @garazdawi, which style of sidebar tabs do you prefer? The current ones or:

Screenshot 2025-01-20 at 18 18 14

Because if we want to change them, I'd do it as we thinner the borders. The third one is showing the mouseover state.

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 20, 2025

We need to either bring everything a bit more to the left or the "Pages" a bit more to the right. Sorry for the pixel shaving but it feels this is the type of feedback you would want to give/receive. :)

fixed 🚀

@josevalim
Copy link
Member

Thank you, I am closing shop for today to focus on a new Elixir patch release, I should be back tomorrow. :)

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 20, 2025

alright so it's down to these options @DavidOliver @jonatanklosko @liamcmitchell @wojtekmach @garazdawi

v1 (active -> inactive -> hover state)

@josevalim i used this because i thought you preferred the old design
image

image

v2 (active -> inactive -> hover state)

this was my initial idea which is more inline with body content tabs
image

image

v3 (active -> inactive -> hover state)

same as 2 but without bg color on hover state, bottom border is enough to convey the state (my favorite)
image

image

v3 after all accessibility concerns are addressed in the other open PRs

image

image

I'm leaning towards v3 personally

@josevalim
Copy link
Member

Let's go with v3. Do you already want to go ahead and change its border style in this PR or a future one? Please do not change the font style for now. :) Thank you!

@DavidOliver
Copy link
Contributor

Sorry for the delayed response. I agree the new style looks to work well, and I like that it matches the new style of content tabs. Title case instead of uppercase is also fine, in my opinion.

@hichemfantar, I expect you have focus (as well as hover) styles in place?

Thanks.

@hichemfantar
Copy link
Contributor Author

hichemfantar commented Jan 21, 2025

v3 applied

here's the focus and hover state @DavidOliver

image

@josevalim josevalim merged commit 732c0d0 into elixir-lang:main Jan 21, 2025
4 checks passed
@hichemfantar hichemfantar deleted the improved-sidebar-tabs branch January 21, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants