Skip to content

Use playlist name for playlist page title #5150

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

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented May 22, 2024

Use playlist name for playlist page title

Pull Request Type

  • Feature Implementation

Related issue

#4949 (comment)

Description

Sets document title to playlist title. This is consistent with how we do it for other routes with custom named content, YT, and user expectations.

Screenshots

Screenshot_20240521_231601

Testing

  • Test document title is updated to the playlist title for a given user playlist or remote playlist (either backend)

Desktop

  • OS: OpenSUSE
  • OS Version: TW

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 22, 2024 04:13
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 22, 2024
@PikachuEXE
Copy link
Collaborator

Can we update text for User Playlists.Your Playlists in this PR too? Got inconsistent casing
image

Also we should prevent long titles like existing logic
https://github.com/FreeTubeApp/FreeTube/blob/v0.20.0-beta/src/renderer/components/ft-list-playlist/ft-list-playlist.js#L55-L60

    titleForDisplay: function () {
      if (typeof this.title !== 'string') { return '' }
      if (this.title.length <= 255) { return this.title }

      return `${this.title.substring(0, 255)}...`
    },

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 22, 2024
Copy link
Contributor

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

@kommunarr
Copy link
Collaborator Author

Can we update text for User Playlists.Your Playlists in this PR too? Got inconsistent casing

This one isn't as straightforward. "Your playlists" conveys the playlists you own. "Your Playlists" conveys the section containing the playlists you own.

Also we should prevent long titles like existing logic

How about constraining the user playlist name length (video title length is already restricted by YT)? Can do this in a separate PR if desired.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 22, 2024
@FreeTubeBot FreeTubeBot merged commit 85b01de into FreeTubeApp:development May 22, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 22, 2024
@PikachuEXE
Copy link
Collaborator

This one isn't as straightforward. "Your playlists" conveys the playlists you own. "Your Playlists" conveys the section containing the playlists you own.

No I only mean the one in en_GB is Your playlists but in en_US is Your Playlists
I forgot to ensure the screenshot to include the files names

How about constraining the user playlist name length (video title length is already restricted by YT)? Can do this in a separate PR if desired.

Please do

@kommunarr
Copy link
Collaborator Author

kommunarr commented May 22, 2024

Oh, I didn't catch that. Yeah, English (UK) has a lot of pretty arbitrary differences from English (US). Might honestly be worth replacing the UK file from the US one and manually scanning for place where the words need to be changed.

PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request May 23, 2024
* development:
  Update playlist name with title (FreeTubeApp#5150)
  User playlists as grid (FreeTubeApp#4949)
  Add custom webpack loader to remove unused mimetypes from mime-db (FreeTubeApp#5148)
  ^ Update GH action eps1lon/actions-label-merge-conflict (FreeTubeApp#5034)
  Translated using Weblate (Italian)
  Translated using Weblate (Serbian)
  Translated using Weblate (Estonian)
  Translated using Weblate (Bulgarian)
  Translated using Weblate (Spanish)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Chinese (Simplified))
  Fix gap next to banner when Hide Side Bar Labels is enabled (FreeTubeApp#5120)
@absidue
Copy link
Member

absidue commented May 23, 2024

@PikachuEXE @jasonhenriquez That string is correct.

Using title case is an American English thing, British English uses sentence case a lot more.
The real problem is actually a different one, the string being discussed here is correct but other ones in English (UK) that use title case, should be corrected to sentence case (presumably they were just copied from English (US)).

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.

6 participants