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

Improve create & edit playlist UX #5226

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Jun 4, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

  • Show empty playlist name message sooner (when name input instead of clicking create button)
  • Make it impossible to create playlists with spaces wrapped name
  • Show duplicate playlist message sooner (when name input instead of clicking create button)
  • Update default new playlist name to include channel name when copying a remote playlist
  • Fixed playlist page showing text in edit mode
  • Updated playlist page edit mode to show messages for empty/blank/duplicate input playlist name
    It should not show error message when got same name as self (when you press edit and changed nothing)

Screenshots

image
image
image
image

Testing

  • ensure cannot create playlist if playlist name blank (with spaces only) (and got message shown)

  • Input spaces around playlist name inside create playlist prompt

  • ensure created playlist got name trimmed

  • ensure cannot create playlist if another playlist with same trimmed name exists (and got message shown)

  • Copy local playlist, create playlist - unchanged

  • Copy remote playlist, create playlist - ensure channel name included

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 4, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 4, 2024 01:35
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Looks good!

suggestion (non-blocking): Could you give the same treatment for the empty playlist name case?

@PikachuEXE
Copy link
Collaborator Author

Could you give the same treatment for the empty playlist name case?

Just did & updated OP

kommunarr
kommunarr previously approved these changes Jun 4, 2024
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Thanks Pika. It's a tiny bit jarring having the empty playlist alert showing from the jump now, but maybe can switch to using the @disabled-click + toast route for some of our ft-button cases. For now, this looks good.

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 6, 2024

Thanks Pika. It's a tiny bit jarring having the empty playlist alert showing from the jump now

Yep i agree. Playlist name cannot be empty. Please input a name. gives me as the user the impression that i did something wrong here but i didnt even had the chance to input something and its already telling me it cant be empty.

I suggest (maybe this isnt a good suggestion idk) that the first time the users gets to see that message is when they input something but remove the input. This way the user doesnt see it right away but does get to see it if they intentionally removed it

Update default new playlist name to include channel name when copying a remote playlist

I like this very much but im not sure how this is relevant when copying random user playlists. Not sure if this is an issue though

VirtualBoxVM_lFnHULW9wA.mp4

ensure created playlist got name trimmed

Should also probably happen here

VirtualBoxVM_ASY528ofty.mp4

FUTURE IDEA

Show empty playlist name message sooner (when name input instead of clicking create button)
Make it impossible to create playlists with spaces wrapped name
Show duplicate playlist message sooner (when name input instead of clicking create button)

This should also be done for creating new profiles

@PikachuEXE
Copy link
Collaborator Author

Updated to only show empty playlist name when space input
image
image

I like this very much but im not sure how this is relevant when copying random user playlists.

Not relevant, it should only work on remote playlist (with channel name present)

Should also probably happen here

I already got :value="playlistName" on ft-input I got no idea why it won't auto update

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 8, 2024

Not relevant, it should only work on remote playlist (with channel name present)

I dont mean Local created playlist. I mean a random YT user created a playlist of your favorite creator.

@PikachuEXE
Copy link
Collaborator Author

a random YT user created a playlist of your favorite creator

I see, but in that case it's still from that YT user not from the creator(s)
Which is why that's the default value but allows editing
Also what happens when the playlist is for 2+ creators? It's really up to the user to decide what's best for them
I have no idea how to programmatically determine for different users on different playlists about whether the channel name is relevant or not...

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

Its indeed not that big of an issue but i thought i'd bring it up in case somebody thinks otherwise

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 10, 2024

Should also probably happen here

I already got :value="playlistName" on ft-input I got no idea why it won't auto update

Everything LGTM aside from this behaving weird.

@PikachuEXE
Copy link
Collaborator Author

Everything LGTM aside from this behaving weird.

Just fixed

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

spacebar isnt working for me anymore

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 11, 2024
@PikachuEXE PikachuEXE force-pushed the feature/playlist/create-playlist-ux branch from faa05cd to aa774c5 Compare June 11, 2024 07:20
@PikachuEXE
Copy link
Collaborator Author

Reverted, no idea what Should also probably happen here means
Oh wait you mean the edit playlist info place? OOF

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

Oh wait you mean the edit playlist info place

yes thats correct, sorry for being vague i thought the screenrec was enough

@PikachuEXE
Copy link
Collaborator Author

  • Fixed create button not disabled when input name blank (spaces only but not empty)
  • Fixed playlist page showing text in edit mode
  • Updated playlist page edit mode to show messages for empty/blank/duplicate input playlist name

@PikachuEXE PikachuEXE changed the title Improve create playlist UX Improve create & edit playlist UX Jun 11, 2024
@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jun 11, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 11, 2024

Fixed playlist page showing text in edit mode

Im not sure what you mean by this. Could you elaborate on this.

I think I wasnt clear enough about what change I would like to see in edit mode.

What I would like to see is the following, trim spaces in edit mode like how its been done in the toast message, the playlist page and inside the playlist

VirtualBoxVM_83YMuWWXCv.mp4

@PikachuEXE
Copy link
Collaborator Author

Fixed playlist page showing text in edit mode

Oh forgot to include screenshot, before:
image
after:
image

What I would like to see is the following

Yes I understand please test the latest version updated recently (though let me know if I miss anything coz create/edit playlist got some logic slightly different)

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 11, 2024

Retested, still seeing the spaces here after i created the playlist

VirtualBoxVM_Qd23ruikXx.mp4

@PikachuEXE
Copy link
Collaborator Author

I don't quite understand what you expect

If you are referring to what's displayed and the actual value (with 2+ spaces inside) that seems to be existing behaviour (not saying it's not a bug but not trying to handle it in this PR)
image

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

Ah you're correct this is existing behavior. Im so confused rn but ill re-review all the changes for the sake of my sanity

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 11, 2024

@PikachuEXE
Bug: Its telling the user it cant insert duplicate or empty names but when user hits the Enter button the playlist still gets created

VirtualBoxVM_lV8V8TRdIT.mp4
VirtualBoxVM_7hMvrfjxXJ.mp4
VirtualBoxVM_YNKyaT3uYp.mp4

@PikachuEXE PikachuEXE mentioned this pull request Jun 12, 2024
1 task
@PikachuEXE
Copy link
Collaborator Author

Sorry I totally forgot enter can do so many things
Also @click isn't the best event name to be linked with pressing enter
Fixed

<ft-flex-box>
<ft-button
:label="$t('User Playlists.CreatePlaylistPrompt.Create')"
:disabled="playlistPersistenceDisabled"
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought/question: I have no idea how Vue knows to make the underlying button disabled when there doesn't seem to be a prop for this in our ft-button class, but it does.

Copy link
Member

Choose a reason for hiding this comment

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

Vue calls that feature "fallthrough attributes", if the component doesn't have a prop for the attribute, it adds the attribute to the root/top-level HTML element inside that component.

Vue 3 has an entire docs page on it, but it also describes Vue 3 specific behaviour so it's not that useful for us yet, Vue 2's docs only have an offhand mention of it (https://v2.vuejs.org/v2/api/#inheritAttrs).

@@ -33,26 +33,45 @@ export default defineComponent({
newPlaylistVideoObject: function () {
return this.$store.getters.getNewPlaylistVideoObject
},

playlistNameEmpty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: once the Vue 3 migration is complete, this will be a great place for composable functions.

@FreeTubeBot FreeTubeBot merged commit efa580c into FreeTubeApp:development Jun 13, 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 Jun 13, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc deleted the feature/playlist/create-playlist-ux branch June 13, 2024 18:05
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.

5 participants