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

Add icon button visual labels #4950

Draft
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Apr 14, 2024

Add icon button visual labels

Pull Request Type

  • Feature Implementation

Related issue

closes #4259

Description

I've had this sitting on the backburner since November. This PR makes many of our ft-icon-buttons, particularly in watch-video-info and playlist-info, have visual labels. This makes it far easier to understand many of our non-obvious icon actions. This problem is less apparent to us contributors of the project who are by now very familiar with these buttons' usage, but I expect this will be very useful to most everyone else.

Screenshots

Screenshot_20240414_171252
Screenshot_20240414_171231

Testing

  • Confirm that icon button labels have not been added in places where they shouldn't be
  • Confirm that there are no visual oddities with these labels in other languages (e.g., Russian, German, Arabic)
  • Test the User Playlist list view and the video view on desktop, tablet, and mobile device sizes

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

  • We're currently trying to display all of the words we can fit in to a comfortable size. We can change this to an implementation where we have a "short version" of the label for this case, that defaults to the first word of the label if none is set (through Western-centric checking for whitespace). This would look cleaner & more similar to the YT implementation, but on the other hand, I'm not sure it yields enough of a benefit compared to the current implementation.
  • I have this PR as a draft because I imagine there will be some discussion about this. I don't intend to start any controversy, so I will keep this drafted until it's revised to a point where we're sufficiently on the same page as a team to move forward with it.

Note that this is disabled for icons that are more universally familiar like the Favorites, More Options, Hamburger, and Refresh icons.
@kommunarr kommunarr added the PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. label Apr 14, 2024
@absidue
Copy link
Member

absidue commented Apr 15, 2024

My biggest concern is narrow screens and mobile devices. they are already struggling to display our current desktop centric UI, so don't have the space to have buttons that are at least 3 times wider (in some languages they could be even larger) than they currently are. Maybe on narrow screens we should hide the labels, to make the buttons look the same as they do before this pull request?

@kommunarr
Copy link
Collaborator Author

Maybe on narrow screens we should hide the labels, to make the buttons look the same as they do before this pull request?

I hesitate to go for this solution because mobile users also greatly benefit from labels for icon buttons, and apps like YT Mobile still have icon labels even given the space considerations because of it. If you're willing to try the YT app on a mobile phone in English or German, you can see what I mean. When it wraps over on the video info page, it becomes a horizontal scroll region. We do the same in this PR, minus the same "prettiness" of the invisible scrollbar. Ours is even notably less wide by a decent margin compared to YT's on German, although that is mostly because we're currently showing the Add to Playlist, Quick Bookmark, Open in External Video Player, and Share buttons as icons still, which I'm still on the fence about (at least for the Share button).

Screenshot_20240415_072645
Screenshot_20240415_073157

@kommunarr
Copy link
Collaborator Author

Some other notable aspects are that YT is able to fit in the full word "Herunterladen", where as we only fit 9 / 13 characters in. I could try messing around with the icon size and button radius to fit more content in.

More differences: YT tries to have single-word labels. The icons all have a minimum size, but if they need more space, they're given it instead of ever cutting the label short like we do (that's in fact why they can fit "herunterladen"). Contrast with us, who have multi-word labels on icon buttons all of equal size (for visual uniformity), and we also break up a single word across two lines mid-character if it can't fit on the first line. I think there are trade-offs of each of our implementations, but ultimately, I do think ours is better suited for the scenario of a trying to maximize utilization of the existing labels for users of all languages. It does look somewhat uglier due to those reasons though, as well as the presence of ellipses. I am still on the fence about a middle ground solution to this; see discussion in the OP:

We're currently trying to display all of the words we can fit in to a comfortable size. We can change this to an implementation where we have a "short version" of the label for this case, that defaults to the first word of the label if none is set (through Western-centric checking for whitespace). This would look cleaner & more similar to the YT implementation, but on the other hand, I'm not sure it yields enough of a benefit compared to the current implementation.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Apr 15, 2024

I like that the buttons will have visual labels and i think we shouldnt hide it on narrow screens. Im leaning towards showing the whole word the best we can if if its starting to get ugly, cut it off

@PikachuEXE
Copy link
Collaborator

If there is an option to disable it then it's fine for me
The screen looks too busy with all the labels present and longer label text = good luck

@efb4f5ff-1298-471a-8973-3d47447115dc

If there is an option to disable it then it's fine for me

Maybe merge option Hide Labels in Sidebar with this one to prevent another toggle

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kommunarr
Copy link
Collaborator Author

I like Piped's icon buttons. As in, just regular buttons with an icon attached, something I added in #4231. Thoughts on that?

Screenshot_20240423_154840

@efb4f5ff-1298-471a-8973-3d47447115dc

That also looks good. I have no real preference so choose the easiest option :)

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. PR: merge conflicts / rebase needed PR: WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Make label visible for most icon buttons
4 participants