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

Fix selection behaviour on the My Downloads page #11397

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 12, 2023

Summary

  • Updates the removeDownloadRequest function to accept a contentnode_id as its only argument (in the process, fixing the latter part of Explore libraries - The option 'Remove from my library' is not positioned correctly and not working #11378)
  • Removes duplication in the responsive view of the My Downloads page to make it easier to reason about
  • Updates the resource selection to be a map from contentnode_id for the request to true to enable easy lookup in the downloadRequestMap and to be compatible with the updated removeDownloadRequest function
  • Adds indeterminate selection determination to the select all checkbox
  • Updates deletion setting to use the new map
  • Updates checked setting to use the new map

References

Fixes #11389
Fixes #11378 - now that the z-index issue has been fixed by @LianaHarris360

Reviewer guidance

Ensure that all selection behaviour behaves as expected:

  • Selecting one should toggle the Select All checkbox to an indeterminate state
  • Selections should be preserved across page navigation, but the Select All checkbox state and effect should be scoped to the current page
  • Clicking on the Select All checkbox in an indeterminate state will Select All on the page
  • Clicking on the Select All checkbox in a checked state will deselect all on the page
  • Clicking on the Select All checkbox in an unchecked state will select all on the page
  • Table should render properly in smaller screen sizes

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 12, 2023
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium labels Oct 12, 2023
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

@rtibbles I confirm that the selection behaviour is fixed now and the option 'Remove from my library' is functioning.
What puzzles me is that now both 'Remove' and 'Remove selected' are not functioning at all:

2023-10-13_11-29-37.mp4

Also when I remove a resource from the library there is no
snackbar 'Resource removed from my library' as specified in Figma:

2023-10-13_11-37-42.mp4

@rtibbles
Copy link
Member Author

Not terribly confusing - I have clearly messed something up :) Thanks for checking my working.

@rtibbles
Copy link
Member Author

Removal should be working everywhere now, and the snackbar should display when removing a download while browsing a remote library.

@pcenov pcenov self-requested a review October 16, 2023 11:07
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Implemented as specified.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, and since this has the Peter stamp of approval, I think we are good to merge. Thanks for the code cleanup and simplification here, @rtibbles :)

@marcellamaki marcellamaki merged commit ad308de into learningequality:release-v0.16.x Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium TODO: needs review Waiting for review
Projects
None yet
3 participants