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

Make learning path items drag resortable #3948

Merged
merged 13 commits into from
Apr 21, 2023

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Apr 19, 2023

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
    • Mobile width screenshots
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#3912

What's this PR do?

This PR makes learning paths drag re-sortable at infinite/lists/<learning path id>.

How should this be manually tested?

  1. Visit http://od.odl.local:8063/infinite/lists
  2. Create and populate some userlists / learning paths if you haven't already. Populate them via the bookmark icon on search results page, http://.od.odl.local:8063/infinite/search. Make sure you have at least one learning path and at least one userlist.
  3. From http://od.odl.local:8063/infinite/lists, visit the details page for a userlist not a learning path. It should not have a "Reorder" button.
  4. Now visit a learning path page. It should have a "Reorder" button. Try reordering things:
  5. Visit a public learning path as an anonymous user in private browser window. You should not be able to Reorder.

Screenshots (if appropriate)

Desktop:

sorting_userlists.mov

Mobile:
Screenshot 2023-04-19 at 11 04 58 AM

@@ -131,84 +139,50 @@ test("useDeleteUserList invalidates all resource queries", async () => {
})

test("useAddToUserListItems invalidates userlist details and listing", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several tests in this file broke when I changed useUserListItems to an infinite paginated query.

I decided the way I was testing "correct stuff gets invalidated by mutation" was too fragile and re-wrote several of those tests:

  • before: I was calling several of the data-fetching hooks, and checking that when useAddToUserListItems (or whatever mutation was under test) was causing some of those hooks to re-fetch.
  • now: I'm just checking that invalidateQueries is called with the correct keys.

It's a little less realistic, but should be much less fragile... don't mock / simulate as much data.

return useQuery<PaginatedUserListItems>(key, () =>
axios.get(url).then(res => res.data)
)
const useUserListItems = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to change useUserListItems to an infinite query (designed for infinite scrolling) because the old learn site uses an <InfiniteScroll /> UI for sorting list items.

I did not implement the InfiniteScroll UI, just set up the API hook to make it easy if we do. The old /learn site uses InfiniteScroll for sorting items on the UserListDetailsPage, but does not paginate the items in read-only mode on that page.

count: number,
{ count, pageSize }: { count: number; pageSize?: number },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

makePaginatedFactory takes a factory function and returns a factory that creates paginated results, { count, previous, next, results }.

Previously count and results.length were always the same, which isn't flexible enough.

@@ -76,6 +76,7 @@
"stylelint": "^15.2.0",
"stylelint-config-standard-scss": "^7.0.1",
"swc-loader": "^0.2.3",
"tiny-invariant": "^1.3.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very small package that is pretty widely used

  • make assertions concisely
  • typescript narrowing

It is similar to assertNotNil and assertInstanceOf, though I probably would not have written those functions if I was familiar with tiny-invariant at the time.

assertNotNil(x) <---> invariant(x)
assertInstanceOf(x, SomeClass) <---> invariant(x instanceof SomeClass)

Comment on lines +322 to +355
* We use the indices to update the UI immediately, and the positions to make
* the API call.
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Apr 19, 2023

Choose a reason for hiding this comment

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

In contrast, the old /learn site uses indices to make API calls.

  • because our list item position field is not updated when list items are deleted, the positions can have gaps, which leads to bug like (open/learn) Re-ordering learning paths does not always work #3944 when re-ordering based on index.

  • Even if we changed the API so that position field does not have gaps... it seems safer to use position field rather than indices to make the API call.

    On the other hand, that means that after making an API call to move an item, we need to either (A) optimistically update the list indices AND the position fields, or (B) make a second API call to re-fetch the list items. I opted for (B).

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review April 19, 2023 15:05
@abeglova abeglova self-assigned this Apr 20, 2023
Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

lgtm

@ChristopherChudzicki ChristopherChudzicki merged commit 84e5ba5 into master Apr 21, 2023
@ChristopherChudzicki ChristopherChudzicki deleted the cc/learningpath-dnd branch April 21, 2023 13:24
@odlbot odlbot mentioned this pull request Apr 26, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants