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

style(tabs): improve visual consistency and label handling for horizontal style #1944

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

DenysMb
Copy link
Contributor

@DenysMb DenysMb commented Oct 24, 2024

Pre-flight Checklist

Please ensure you've completed all of the following.

Description of Change

When we move the sidebar to the top and show the services label, we get a very inconsistent visual.
The service tab width is different according to the size of the service label and the active indicator pushes everything down, so the active service tab keeps unaligned with all the others.

In this PR I fixed both issues by using a fixed width for all the tabs when labels are displayed and using box-shadow instead of a border for the active tab indicator so things are not pushed down, This way we have a more consistent look for all the tabs and the whole application in general.

Another problem is that the long labels were not "ellipsed, I fixed this too.

Motivation and Context

I prefer to use the tabs at the top of the application, but all these little issues prevented me from doing so because they bothered me a lot.

Screenshots

Before:
Screenshot_20241024_181046
Screenshot_20241024_181052

After:
Screenshot_20241024_181224

Checklist

  • My pull request is properly named
  • The changes respect the code style of the project (pnpm prepare-code)
  • pnpm test passes
  • I tested/previewed my changes locally

Release Notes

  • All tabs, when the service label is enabled and the sidebar is on the top, now have the same width, labels get ellipsed and the activated tab is no longer un-aligned with others.

Copy link
Contributor

@vraravam vraravam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vraravam vraravam merged commit e004727 into ferdium:develop Oct 25, 2024
5 checks passed
@vraravam
Copy link
Contributor

@all-contributors please add @DenysMb for code

Copy link
Contributor

@vraravam

I've put up a pull request to add @DenysMb! 🎉

@DenysMb DenysMb deleted the feature/fix-styling-issues branch October 26, 2024 12:20
@wvds
Copy link

wvds commented Nov 12, 2024

@DenysMb @vraravam I think this PR breaks the use of the accent color for the tabs.
Before the change, the accent color was used for the "active" state of the tab, but with this change it's always purple (the default accent color of Ferdium).

@DenysMb
Copy link
Contributor Author

DenysMb commented Nov 12, 2024

@DenysMb @vraravam I think this PR breaks the use of the accent color for the tabs. Before the change, the accent color was used for the "active" state of the tab, but with this change it's always purple (the default accent color of Ferdium).

Very strange, since I am using the SCSS $theme-brand-primary variable that should handle this.
I'll take a look now to see what broke this.

Edit: I was wrong. The accent color is controlled in the appearance/index.ts. I changed from border to box-shadow in SCSS but there still changes the border-color. So, I just need to change there from border-color to box-shadow too and it will work. I'll open a PR for this.

@DenysMb
Copy link
Contributor Author

DenysMb commented Nov 12, 2024

@wvds I just opened a PR to fix this: #1967
Thanks for letting me know. Nobody wants their contribution to cause a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants