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

Draggable list card styles #1282

Merged
merged 6 commits into from
Jul 18, 2024
Merged

Draggable list card styles #1282

merged 6 commits into from
Jul 18, 2024

Conversation

jonkafton
Copy link
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4859

Description (What does it do?)

Applies the list card draggable / sortable designs to the User List and Learning Path item list screens.

  • Provides a draggable prop and applies the draggable design to the ol-components LearningResourceListCard and LearningResourceListCardCondensed.
  • Applies to the ItemsListing when sortable (User List Listing Page, Dashboard User List Details Tab).
  • Removes the @page-components LearningResourceCard and LearningResourceCardTemplate from frontends/mit-open.

Screenshots:

Condensed, desktop:
image

Condensed, mobile:
image

Default, desktop:
image

Default, mobile:
image

How can this be tested?

While logged in:

  • Navigate to these pages, click "Reorder" drag and confirm the draggable list card designs have been applied, https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=9005-102169&m=dev

    • /userlists/ (or click a list from /userlists to navigate to a User List Listing Page
    • /dashboard/#my-lists and click a list to navigate to the Dashboard User List Details Tab
    • /learningpaths/ (or click from a Learning Path in /learningpaths to navigate to a Learning Path Listing Page)
  • Drag to a new position, click "Done ordering" and confirm that the new position is preserved after refresh.

Additional Context

The designs show the draggable list card in condensed mode for desktop (no image, User Lists version). I've added a mobile breakpoint that adjusts the drag handle and surrounding margins to work with the mobile view of the card ("condensed, mobile" above). @steven-hatch these could do with a design review and ideally added in Figma. Let me know if you have changes or if I have missed it in Figma. Same thing for the default larger list card view on the Learning Path Listings page.

Animation on the drag handle area width is challenging as we remove and redraw the list when we enter reordering mode, ie. ItemsListing toggles ItemsListingSortable and StyledPlainList. It's marked as optional, though let's prioritise separately if the design value warrants the complexity.

@jonkafton jonkafton marked this pull request as draft July 17, 2024 21:48
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Jul 17, 2024
@jonkafton jonkafton marked this pull request as ready for review July 17, 2024 21:54
@mbilalmughal
Copy link

Great work, @jonkafton! I really like it 👍. The mobile card spacing is adjusted perfectly.
CC: @steven-hatch

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki 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! I did notice one style issue, but it isn't new.

The intent is:

  • draggable has cursor: pointer ... this works fine
  • dragging has cursor: grab ... this does not

I believe the issue is in frontends/ol-components/src/components/SortableList/SortableList.tsx near

<div className="ol-dragging">{active && renderActive(active)}</div>

Might be worth fixing here.

Comment on lines +140 to +142
<Loading>
<LoadingSpinner loading />
</Loading>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace this with skeletons? Guess it could be weird to show 10 skeletons and then render a list with 2 cards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes definitely, though I thought it out of the scope of this task.

Copy link
Contributor

Choose a reason for hiding this comment

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

👏

${containerStyles}
display: flex;
`

const Content = () => <></>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? I know it's not new, failed to notice it earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DragArea and Body are siblings that need a flex container to place items adjacently. It happens that the LinkContainer is already a flex container, but we don't use this while in draggable mode (we don't want hover states or click to navigate), so here I'm adding a Container for dragging that displays flex.

@jonkafton jonkafton merged commit 4a4ca34 into main Jul 18, 2024
11 checks passed
@jonkafton jonkafton deleted the jk/4859-list-cards-sorting branch July 18, 2024 16:30
@odlbot odlbot mentioned this pull request Jul 18, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants