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.

Sign up for GitHub

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

Jump to bottom

fix: sort playlist feature bug #415

Merged
merged 3 commits into from
Nov 30, 2020
Merged

fix: sort playlist feature bug #415

merged 3 commits into from
Nov 30, 2020

Conversation

compiuta
Copy link
Contributor

Closes #411

What does this PR do / solve?

Currently the sort playlist feature is using the index and paylistname data attributes to sort by date and alphabet. The issue is that I did not realize that the array sort function takes into consideration lower and upper case when sorting.

Also the index is being sorted as a string rather than a number which causes the playlist to be in the incorrect order when sorting by date.

Overview of changes

The sort function will now lower case all playlist titles and convert index strings into numbers before sorting to prevent these bugs.

How to test this PR?

If you currently sort the playlist with a user who has more than 10 playlists https://openwhyd.org/adrien/playlists the inspector shows an unexpected sort behavior.

After this update the sort by date will sort the playlist by the data-index attribute by ascending order:
Screenshot from 2020-11-29 08-30-47

When using the alphabetize sort option the playlist will now correctly get ordered using the data-playlistname as expected regardless if the title is capitalized:
Screenshot from 2020-11-29 08-31-17

@compiuta compiuta changed the title (bug) fix sort playlist feature fix: sort playlist feature bug Nov 29, 2020
adrienjoly
adrienjoly approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@adrienjoly adrienjoly left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thank you!

EDIT: I also made sure that Cypress tests pass, on my developer environment.

@adrienjoly adrienjoly added the bug label Nov 30, 2020
@adrienjoly adrienjoly added this to 📥 Inbox / ideas in Development via automation Nov 30, 2020
@adrienjoly adrienjoly removed this from 📥 Inbox / ideas in Development Nov 30, 2020
@adrienjoly adrienjoly merged commit aec724d into openwhyd:master Nov 30, 2020
adrienjoly pushed a commit that referenced this pull request Nov 30, 2020
@semantic-release-bot
chore(release): 1.44.2 [skip ci]
4270244 
## [1.44.2](v1.44.1...v1.44.2) (2020-11-30)

### Bug Fixes

* **ui:** Sort playlist feature bug ([#415](#415)) ([aec724d](aec724d)), closes [#411](#411)
@adrienjoly adrienjoly mentioned this pull request Nov 30, 2020
Closed
@compiuta
Copy link
Contributor Author

compiuta commented Dec 1, 2020

Sounds good 👍

@compiuta compiuta deleted the sort-playlist-bug-fix branch December 1, 2020 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrienjoly adrienjoly adrienjoly approved these changes

Assignees

@compiuta compiuta

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(bug) sort playlist not working as expected
2 participants
@compiuta @adrienjoly