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

Edit playlist UI improvements #602

Merged
merged 51 commits into from
Aug 1, 2019
Merged

Edit playlist UI improvements #602

merged 51 commits into from
Aug 1, 2019

Conversation

ofsound
Copy link
Collaborator

@ofsound ofsound commented May 17, 2019

Most of the changes are figma matching the bottom playlist track sorting/dragging layouts.

This is a branch that has a few 'placeholder' values for dynamic rails data (total tracks, total length).

I think the 'your uploads', 'your listens' buttons have yet to be hooked up in WT world.

Also, the ajax 'added' etc. are floating free. Perhaps the visual feedback of the tracks showing up and disappearing are enough, or those would fit in the '0 tracks by montgomeru' box at the bottom of the left column.

"Ready For Review" Checklist

  • PR title accurately summarizes changes
  • PR description
    • Does this fix any open issues?
    • What changes are you making?
    • Why did you implement it this way?
    • Is there anything reviewers need to know or pay attention to?
    • Does anything special need to happen for deployment?
  • Optional: Inline github PR comments explaining context of tricky / complicated / unexpected lines so reviewers can follow along.
  • If appropriate, new tests were added for isolated methods or new endpoints
  • All tests green
  • Percy changes are purposeful or explained
  • I booted up the branch locally, exercised any new code I wrote.
  • If css was touched, I verified changes are happy on mobile (via Percy is ok)
  • I opened an issue for any logical followups

@ofsound ofsound requested a review from sudara May 17, 2019 22:55
@sudara sudara changed the title Edit playlist page Edit playlist UI improvements Jun 22, 2019
@sudara
Copy link
Owner

sudara commented Jul 15, 2019

@ofsound If you have a few moments, could you take a look at this one and help me figure out what's next with it?

@ofsound
Copy link
Collaborator Author

ofsound commented Jul 16, 2019

@ofsound If you have a few moments, could you take a look at this one and help me figure out what's next with it?

Hello @sudara

Looks like the main functionality to add is to have the 'your listens' 'favorites' buttons load new stacks of tracks into the right column.

available_tracks_total is still placeholder text, I imagine that would udpate with those buttons being clicked.

We should discuss the "Added" "Removed" flash messages, if those could use our new green check style? Or how they would interact with the '5 tracks by montgomeru' gray bar at the bottom.

I think a round of testing for the adding and removing Stimulus functions is needed, getting some errors on removal about this.playlistEdit.feedbackTarget.toggle. There are Xs that show up on the assets in the right column, and it doesn't take too many clicks to get a 'Dang something went wrong.'

@sudara
Copy link
Owner

sudara commented Jul 30, 2019

@ofsound Ok, your turn!

The main functionality to add is to have the 'your listens' 'favorites' buttons load new stacks of tracks into the right column.

As discussed I hid these for now.

available_tracks_total is still placeholder text, I imagine that would udpate with those buttons being clicked.

Still todo...

We should discuss the "Added" "Removed" flash messages, if those could use our new green check style? Or how they would interact with the '5 tracks by montgomeru' gray bar at the bottom.

Using green checkmark now, can you look @ z-index and shadows?

There are Xs that show up on the assets in the right column, and it doesn't take too many clicks to get a 'Dang something went wrong.'
Is this still the case for you? Looks like there was some bad js in there until now, should be cleaned up.

More things:

  1. Can you cook up a "That didn't work" version of the flash with red X and red border? Also, it looks like the transparency of the flash message is working against us, it's looking pretty noisy.

image

  1. I'd like to try and keep our spinner, maybe putting it near the info about number of tracks, would you mind repositioning?

alonetone 2019-07-30 02-51-13

  1. I still need to make the pagination ajax-y, as currently I think we have some scroll position issues when we reload a new page paginating...

@sudara
Copy link
Owner

sudara commented Jul 30, 2019

@ofsound Think I'm done with most of my tasks...

This textarea might need some love:

image

I guess we might want to let people remove playlist covers instead of just replacing them? Do you have any ideas there?...

…ar at the bottom of playlist edit left column box
@ofsound
Copy link
Collaborator Author

ofsound commented Jul 30, 2019

@sudara

I guess we might want to let people remove playlist covers instead of just replacing them? Do you have any ideas there?...

Maybe a plain link below "Upload a square photo, 2000px by 2000px for best results."

Remove Uploaded Artwork

I think they'd find out quickly it returns them to stain glass world?

That was my use case, "eh, i dunno, i'd rather just have the stained glass for now until i figure this out"

@ofsound
Copy link
Collaborator Author

ofsound commented Jul 30, 2019

@sudara

What about a View (Return to?) Playlist... Save and View Playlist link somewhere?

@sudara
Copy link
Owner

sudara commented Jul 31, 2019

@ofsound Probably a good idea! I'll do that as well as getting a Percy shot of the edit page.

Any idea why percy thinks there's more spacing below comment submit buttons?

@sudara sudara marked this pull request as ready for review August 1, 2019 17:34
@sudara sudara merged commit bfb86f6 into master Aug 1, 2019
@sudara sudara deleted the edit_playlist_page branch August 1, 2019 20:36
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.

None yet

2 participants