Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Oct 15, 2024

What are the relevant tickets?

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

Description (What does it do?)

Screenshots (if appropriate):

Change: The AddToListDialog shows more lists and is scrollable (if necessary):

Screenshot 2024-10-15 at 11 06 53 AM

Unchanged: Most other dialogs

Screenshot 2024-10-15 at 11 07 03 AM Screenshot 2024-10-15 at 11 07 15 AM Screenshot 2024-10-15 at 11 07 26 AM Screenshot 2024-10-15 at 11 07 40 AM

How can this be tested?

  1. Create a bunch of learning paths (or userlists) and check that the AddToList dialog content scrolls. (You can open the AddToListDialog via list/bookmark button on learning resources).
  2. Check that other dialogs, like the list creation (in dashboard -> my lists) is unchanged

const resourceQuery = useLearningResourcesDetail(resourceId)
const resource = resourceQuery.data
const listsQuery = useLearningPathsList()
const listsQuery = useLearningPathsList({ limit: LIST_LIMIT })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Showing up to 100 and scrolling isn't a great solution. We should have infinite scroll or something.

This is a remediation to unblock the DLLs.


const Content = styled.div`
margin: 28px 28px 40px;
margin: 28px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, in Figma, a couple dialogs had 40px between the content and the dialog footer but most had 28px.

In THE CODE it was determined via Dialog vs FormDialog. Both used 40px margin on the content, but FormDialog also had -12px on the footer for an effective 28px. This is (a) weird, and (b) the negative margin approach did not work well with scrolling overflow. (Which is why it's showing up in this PR).

Confirmed with @steven-hatch we want 28px uniformly across the dialogs.

Here's an example of a noticeable change:

Screenshot 2024-10-15 at 11 17 04 AM

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review October 15, 2024 15:22
@shanbady shanbady self-requested a review October 15, 2024 15:23
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

👍

@ChristopherChudzicki ChristopherChudzicki merged commit 1c18058 into main Oct 15, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Oct 15, 2024
4 tasks
@rhysyngsun rhysyngsun deleted the cc/add-to-list-scrolling branch February 7, 2025 20:38
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.

3 participants