Skip to content

Make video info section more concise#4338

Merged
FreeTubeBot merged 6 commits intoFreeTubeApp:developmentfrom
kommunarr:feat/more-concise-video-info
Feb 1, 2024
Merged

Make video info section more concise#4338
FreeTubeBot merged 6 commits intoFreeTubeApp:developmentfrom
kommunarr:feat/more-concise-video-info

Conversation

@kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Nov 15, 2023 •
edited
Loading

Make video info section more concise

Pull Request Type

  • Bugfix
  • Feature Implementation

Related issue

closes #4336

Description

  • Puts title on its own row
  • Puts video info section on its own row
  • Puts subscribe button & video buttons on their own row
  • Fixes bug of videoOptions buttons overflowing the video info section that occurred on some viewport widths

Video

simplescreenrecorder-2023-09-20_19.22.02-2023-11-15_09.45.57.mp4

Edit: modified the publish date, # of views, and likes to be 1.5 pts larger. Also narrowly reduced the margin between the subscription button and title.

Screenshot_20231115_170031

Testing

  • Ensure that appearance is great at all screen sizes
  • Ensure that the video options buttons no longer extend past the container when resizing viewport

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 15, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 15, 2023 15:53
absidue
absidue reviewed Nov 15, 2023
View reviewed changes
src/renderer/components/watch-video-info/watch-video-info.scss Outdated
}
}
.videoMetrics {
font-size: 12px;
Copy link
Member

@absidue absidue Nov 15, 2023

Choose a reason for hiding this comment

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

Please choose a larger font size, 16px is considered the minimum that is readable, especially as the items, that are now on that row, used to be 16px and 15px before (stylelint used to complain about that but afaik you disabled everything, including that check, when you did you block and inline size rules).

Copy link
Collaborator Author

@kommunarr kommunarr Nov 15, 2023

Choose a reason for hiding this comment

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

16px is considered the minimum that is readable

Is it okay if I ask what your source is for this claim? The closest I can find is this article recommending 16 pixels as optimal for mobile, which ends up being fine here as well, as device manufacturers use the reference px size to set the actual rasterizing size based on intended distance of viewing. Not to uphold YouTube as a superior example of things, but they themselves use 12px font for the "1.46M subscribers" text in the image below. For the # of views and likes, they use 14px font. From my research, 12px seems to be the consistent minimum recommended font size (16px should generally be used as the minimum for body text, and 12px is acceptable for supplementary information). If we had a stylelint rule that disallowed font sizes below 16px (which seemingly wasn't being enforced, as I see dozens of instances of 12px and even 11px font-sizes in our codebase), I personally think that's unnecessarily stringent. It's useful in places like here for demarcating levels of importance of information and guiding the eye to salient focal points.

Screenshot_20231115_153641

Copy link
Collaborator Author

@kommunarr kommunarr Nov 15, 2023

Choose a reason for hiding this comment

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

I did increase it to 14px to incorporate your feedback, though, because I think you can achieve that effect here even at that size. I do appreciate the feedback (I'm not a bikeshedder!). I just think 16px or higher for this text draws a jarring level of attention to it compared to the surrounding text, which is why I see the other platforms opting to make this text 14px or smaller.

Screenshot_20231115_160051

Copy link
Member

@absidue absidue Nov 15, 2023

Choose a reason for hiding this comment

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

The stylint checks were optional for so long because we had many different violations in the codebase. Anyway here is the markdown document associated with that accessibility rule:

https://github.com/double-great/stylelint-a11y/tree/main/src/rules/font-size-is-readable

Copy link
Collaborator Author

@kommunarr kommunarr Nov 15, 2023

Choose a reason for hiding this comment

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

The articles cited there in the justification for the rule are the MDN article for font-size (which doesn't seem to broach the topic of recommended minimum size) and this article talking about body text. When we add this rule back, we should definitely consider adding it as a warning, as there are quite a few exceptions to that rule.

@kommunarr kommunarr requested a review from absidue November 15, 2023 23:01
@kommunarr kommunarr force-pushed the feat/more-concise-video-info branch from 09986e6 to 17e826b Compare November 15, 2023 23:03
@kommunarr kommunarr mentioned this pull request Nov 18, 2023
Merged
1 task
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc previously approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

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

Choose a reason for hiding this comment

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

LGTM

@PikachuEXE
Copy link
Member

PikachuEXE commented Nov 24, 2023 •
edited
Loading

I find it weird to see like number+icon left alone on the right now
Feels like it can be moved to video info row

image

@kommunarr
Copy link
Collaborator Author

kommunarr commented Nov 24, 2023

@PikachuEXE Thank you, will consider it if others prefer as well - that is YouTube's approach that was considered for the main issue, but I don't really have a super strong preference

@PikachuEXE
Copy link
Member

PikachuEXE commented Nov 24, 2023

It's not blocking especially if it's following YT
Just feel like it look strange now especially with wide window

@github-actions
Copy link
Contributor

github-actions bot commented Dec 23, 2023

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2024

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.

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 30, 2024

Maybe the breakpoint can be set at a greater width?
I see this with 901px width
image

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 30, 2024

@PikachuEXE The breakpoint at which "Up Next" is put on its own level?

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 30, 2024

Sorry for being unclear, it's the buttons now arranged (squeezed) into a square

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 30, 2024 •
edited
Loading

Are you talking about setting it to wrap into a box earlier? If you mean the other way around, that is difficult to do without dynamically shrinking the button size, as this is just the flex behavior. What happens when it doesn't compress is this existing bug mentioned in the 4th bullet point in the description (taken from nightly):

Screenshot_20240130_171950

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

Are you talking about setting it to wrap into a box earlier?

As I said Maybe the breakpoint can be set at a greater width? (e.g. 1000px)

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 31, 2024 •
edited
Loading

Sorry, I'm having trouble parsing what you mean. It's flex, so it's just wrapping dynamically based off of the space available.

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

I mean the breakpoints for different panels in watch page (src/renderer/views/Watch/Watch.scss)
(Probably replace all min-width: 901px w/ min-width: 1001px

1001px
image
999px
image

Also I think this should be rebased/merged with dev to be able to test probably due to new buttons

@kommunarr kommunarr force-pushed the feat/more-concise-video-info branch from 17e826b to 84afad6 Compare January 31, 2024 02:10
@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 31, 2024

Ah, I am terrible at reading, it seems! Increased it to 1000px from 900px.

Also I think this should be rebased/merged with dev to be able to test probably due to new buttons

Did this as well now

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

A few more pixels would be great :)
at 1005px:
image

@kommunarr kommunarr force-pushed the feat/more-concise-video-info branch from 84afad6 to 6f8d1cc Compare January 31, 2024 02:23
@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 31, 2024

Blerg, good callout. I just made it 1050px to compensate for other languages with a longer word for "Unsubscribe" or "Subscribe"

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

community-post updated too?

@kommunarr kommunarr force-pushed the feat/more-concise-video-info branch from 6f8d1cc to 45c135b Compare January 31, 2024 03:15
@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 31, 2024

I modified the wrong likeCount from a different component & didn't realize it because I added the change in my local template. Not my brightest day...

PikachuEXE
PikachuEXE previously approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Look good at 105x px

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

Oh wait, I enabled theatre mode and the breakpoint is not updated for it...
977px:
image

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jan 31, 2024

And it's even narrower if the sidebar is fully expanded. I don't think we can fully prevent the button container from wrapping early under these scenarios without picking an excessively large breakpoint

@PikachuEXE
Copy link
Member

PikachuEXE commented Jan 31, 2024

I am saying that the breaking point for theatre mode should also be 1050px (since breakpoint for non theatre mode updated)
(The one with &.useTheatreMode { & @include theatre-mode-template;)
1051px:
image
1047px:
image

@FreeTubeBot FreeTubeBot merged commit ad9cabd into FreeTubeApp:development Feb 1, 2024
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 1, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Feb 2, 2024
@PikachuEXE
Merge branch 'development' into custom-builds/current
e372f51 
* development:
  Make video info section more concise (FreeTubeApp#4338)
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Feb 3, 2024
@PikachuEXE
Merge branch 'development' into feature/auto-load-next-page
9861ad2 
* development: (92 commits)
  Make video info section more concise (FreeTubeApp#4338)
  Playlist performance improvements (FreeTubeApp#4597)
  ! Fix playlist type not passed when playing next/prev item in a user playlist (FreeTubeApp#4623)
  Properly localize playlist view and video counts (FreeTubeApp#4620)
  Translated using Weblate (Croatian)
  Translated using Weblate (German)
  Translated using Weblate (Croatian)
  Fix search bar handling of Invidious channel URLs (FreeTubeApp#4568)
  Local API: List related games in featured channels section (FreeTubeApp#4562)
  Workaround community post slider dependency incorrectly calculating its size (FreeTubeApp#4598)
  Add support for viewing movie trailers with local api (FreeTubeApp#4391)
  Bump the eslint group with 2 updates (FreeTubeApp#4616)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Bump electron from 28.1.4 to 28.2.0 (FreeTubeApp#4611)
  Translated using Weblate (French)
  Bump the eslint group with 4 updates (FreeTubeApp#4581)
  Bump lefthook from 1.6.0 to 1.6.1 (FreeTubeApp#4608)
  Bump marked from 11.1.1 to 11.2.0 (FreeTubeApp#4612)
  Bump webpack from 5.89.0 to 5.90.0 (FreeTubeApp#4610)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@absidue absidue Awaiting requested review from absidue

@MarmadileManteater MarmadileManteater Awaiting requested review from MarmadileManteater

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request]: More concise video info formatting

6 participants

@kommunarr @PikachuEXE @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer @FreeTubeBot