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.

Sign up for GitHub

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

Fix misaligned tab list indentation on mobile #4610

Closed
wants to merge 15 commits into from
Closed

Fix misaligned tab list indentation on mobile #4610

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d0330b7
fix: fix indentation for mobile view only
pat-s Dec 22, 2024
4d2471c
use pl-2.5 class
pat-s Dec 22, 2024
c6e406e
underline instead of chevron
pat-s Dec 27, 2024
5b74df1
Merge branch 'main' into chore/fix-identation-tabs-mobile
pat-s Dec 27, 2024
2ff0660
remove chevron-right from panel
pat-s Dec 27, 2024
33efe19
rev panel change
pat-s Dec 27, 2024
0ba19bd
only underline exact avtive
pat-s Dec 27, 2024
b3f0356
[pre-commit.ci] auto fixes from pre-commit.com hooks [CI SKIP]
pre-commit-ci[bot] Dec 28, 2024
868eb60
Merge branch 'main' into chore/fix-identation-tabs-mobile
pat-s Dec 28, 2024
49f738b
replace underline with bg highlighting, add padding
pat-s Dec 28, 2024
51df10b
always center
pat-s Dec 28, 2024
39237be
fix bg color, add border-radius
pat-s Dec 28, 2024
a9a4495
add px-2 for hover
pat-s Dec 29, 2024
a67b7b5
only apply highlighting to mobile
pat-s Dec 29, 2024
abdcad5
hover padding
pat-s Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions web/src/components/layout/scaffold/Tabs.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
<template>
<div class="flex flex-wrap mt-2 md:gap-4">
<div class="flex flex-wrap md:gap-4 mt-2">
<router-link
v-for="tab in tabs"
:key="tab.title"
v-slot="{ isActive, isExactActive }"
:to="tab.to"
class="border-transparent w-full py-1 md:w-auto flex cursor-pointer md:border-b-2 text-wp-text-100 items-center"
:active-class="tab.matchChildren ? '!border-wp-text-100' : ''"
:exact-active-class="tab.matchChildren ? '' : '!border-wp-text-100'"
class="flex items-center px-2 py-1 border-transparent md:border-b-2 w-auto w-full md:w-auto text-wp-text-100 cursor-pointer"
Copy link
Member

Choose a reason for hiding this comment

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

w-full and w-auto should not be used together and as I said already, I'm strictly against not using full width on mobile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying this out, I think now that using full-width on mobils is a mistake.
It forces the tabs to be sorted below each other, by this using up a lot of vertical screen space and leaving a lot unused. In addition, a full width background highlighting of the active one looks really odd.

By restricting width, the tabs are aligned horizontally. To me, this makes much better use of the available space.

This is a mobile screenshot:

image

Copy link
Member

@xoxys xoxys Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry but then at least I will not approve it. It looks ugly, is pretty uncommon, and its difficult to hit the correct button on small screens:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you name an argument why the vertical variant would be better? I can't find any.

The colored space is fully unused for no reason. It covers 60% of the default horizontal screen space on mobile.

image

Copy link
Member

@xoxys xoxys Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Im not blind :) Im aware of the spacing issue and yes we can improve the menu with something like a "more" menu on mobile e.g. similar to gh:

image

But that's not that easy and needs more work and is not a topic for this PR. I added my argumentation to my last comment.

Copy link
Member

@xoxys xoxys Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like that we mix in more and more issues and new ideas in existing PRs. If you want to get forward, I would suggest staying with the current layout. Otherwise, it needs to wait until it gets more feedback/approvals from other maintainers. I cant approve it as I think its technically and in terms of UX wrong.

Copy link
Contributor Author

@pat-s pat-s Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see your point and I am also fine to revert to keep this PR small. However, it sometimes/often happens that one stumbles over an issue only when iterating on a specific topic. Then again it might happen that the proposed solution doesn't work so well and including an additional change might round up the picture of the PR.

TBH, I am absolutely unhappy with adding a full-width bg highlighting. In that case, I would have a strong vote in favor of the initially proposed underline.
To find a middleground that suits both, I've started iterating on a "good" bg highlighting approach that would suit both. Yet now we're here in a semi-complicated (meta) discussion.

One option now is to merge the "small" PR variant with the full width bg highlighting but I will open add a follow-up PR then to propose a change to a horizontal layout (which hopefully doesn't get denied then as otherwise I would feel somewhat played out 😅️).
If that is not agreed on, I'd favor to not merge this PR and let it be evaluated again later in a broader scope, once I've finished to come up with such a horizontal solution.

Copy link
Member

@xoxys xoxys Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not a meta discussion. You changed the overall layout fundamentally in the middle of a PR that aims to fix a small alignment issue. I will for sure deny any horizontal layout on mobile 😅 We can make the mobile nav a rollup menu or a popup like github or gitea do or any similar approach but is has to be vertically aligned in any case.

As you like to compare to other well known apps as reference, can you name a single one where the mobile nav looks like the one you proposed? I cant and there is a good reason for it.

Im ok to go with an underline indicator for now, even if the bg indicator isnt that bad...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a meta discussion. You changed the overall layout fundamentally in the middle of a PR that aims to fix a small alignment issue.

Yes, but I didn't hide this change and fully OK in general with the idea of changing a PR scope after creation for a good reason. If this would have been the initial scope, nothing would be wrong about it.
And I'd rather have a PR that ends up in a "working" solution than a partially working one, even if that means extending/delaying the PR. It is not about winning a sprint in merging PRs.
I also think it is pretty normal to discover such things when you touch parts of the code base for the first time (personally) and then find out that XYZ are suboptimal but should ideally be changed together.

I started with wanting to fix the indentation of the chevron-right icon. Then I realized it has no function at all and only exists on mobile and should likely be removed. While at it, I realized there is no indicator which tab is actually active, so I had the idea of the underline. You then brought up the bg highlighting idea, which is good and I tried to add. I then (accidentally) discovered the forced full-width for each of these items and the odd look of the full-width highlight and now we're here.
(I know you're aware of all this, just wanted to outline how the overall process was and that there's always a reason for every step in the process).

We can make the mobile nav a rollup menu or a popup like github or gitea do or any similar approach but is has to be vertically aligned in any case.

I am with you for the admin list as this is one is long and might potentially get even longer. In such cases I am fully with you and would even say that there should be hamburger fold by default (as for GH and GL mobile).

The pipeline list however consists of three fixed items which perfectly fit horizontally without needing folding. I don't see why these must be aligned vertically at all costs with a questionable full-width highlighting. Have you tested the current state directly on your phone? It is much better than before (again, only talking about the pipeline list). We don't need hover on mobile (this still needs to be removed).

PS: Thanks for the calm and positive-minded discussion. I think such discussion are required to achieve (good) improvements in the end. Appreciate it 🤝️

Copy link
Member

@xoxys xoxys Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The pipeline list however consists of three fixed items which perfectly fit horizontally without needing folding.

The menu should work in the same way for all pages. Just because it looks ok on one page doesnt solve the problem. Besides that what happens if more menu items are added? Or someone uses a smaller phone than yours? The approach chosen by GH keeps items vertically aligned as long as they fit in one line, all items that dont fit in this single line are moved into a "more" menu dynamically. As I said, Im fine with such an approach.

We don't need hover on mobile (this still needs to be removed).

Well hover effects apply e.g. on long press actions on mobile. I dont see a reason to remove them, in best case they never appear and can just stay instead of adding more code to remove them.

:active-class="tab.matchChildren ? '!border-wp-text-100 dark:wp-background-100 bg-wp-background-300' : ''"
:exact-active-class="tab.matchChildren ? '' : '!border-wp-text-100 dark:wp-background-100 bg-wp-background-300'"
Copy link
Member

@xoxys xoxys Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

border-radius is missing and it is the wrong color.

>
<Icon
v-if="isExactActive || (isActive && tab.matchChildren)"
name="chevron-right"
class="md:hidden flex-shrink-0"
/>
<Icon v-else name="blank" class="md:hidden" />
<span
class="flex gap-2 items-center md:justify-center flex-row py-1 px-2 w-full min-w-20 dark:hover:bg-wp-background-100 hover:bg-wp-background-200 rounded-md"
Copy link
Member

Choose a reason for hiding this comment

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

px-2 got lost here:

image

class="flex flex-row md:justify-center items-center gap-2 dark:hover:bg-wp-background-100 hover:bg-wp-background-200 py-1 rounded-md w-full min-w-20"
>
<Icon v-if="tab.icon" :name="tab.icon" :class="tab.iconClass" class="flex-shrink-0" />
<span>{{ tab.title }}</span>
Expand Down
8 changes: 4 additions & 4 deletions web/src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ body,
}

.vue-notification {
@apply rounded-md text-base border-l-6;
@apply border-l-6 rounded-md text-base;
}

.vue-notification .notification-title {
Expand All @@ -131,7 +131,7 @@ body,
*::-webkit-scrollbar-thumb {
transition: background 0.2s ease-in-out;
border: 3px solid transparent;
@apply bg-cool-gray-200 dark:bg-dark-200 rounded-full bg-clip-content;
@apply bg-clip-content bg-cool-gray-200 dark:bg-dark-200 rounded-full;
}

*::-webkit-scrollbar-thumb:hover {
Expand All @@ -143,11 +143,11 @@ body,
}

.code-box {
@apply bg-wp-code-200 p-4 rounded-md text-wp-code-text-100 text-sm break-words;
@apply bg-wp-code-200 p-4 rounded-md text-sm text-wp-code-text-100 break-words;
white-space: pre-wrap;
}

.code-box-inline,
code:not(pre > code) {
@apply bg-wp-code-200 rounded-md text-wp-code-text-100 px-1 py-px;
@apply bg-wp-code-200 px-1 py-px rounded-md text-wp-code-text-100;
}