-
Notifications
You must be signed in to change notification settings - Fork 687
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
limit number of cards visible in "Recent" #11515
limit number of cards visible in "Recent" #11515
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @thanksameeelian - nice work! Manual QA checks out and the code read through looks okay. One small request, which is technically outside of scope of this but would be nice to fix, is to add a max-width for wide screens which seems to have been lost somewhere along the way with refactoring. Don't worry about the other places for now, but if you could add it for the content cards, that would be great. You can see the description here. My first thought is to add it on the LibraryAndChannelBrowserMainContentGrid
and check if it works for both "recent" on the library page as well as on the topics page. That could give us a lot of bang for our buck. If that gets weird, you can just go ahead and
do it directly on the ResumableContentGrid
and not worry about the other places for now, since this is already a bit of a reach. Thanks! https://www.figma.com/file/uiZuCIqh2KYvBUfbCiJyyA/Hybrid-Learning?type=design&node-id=9-2&mode=design&t=fNsWatcu1iV90BrI-0
After that I'll do a final review and this should be good to go!
…er to match spec, add handling for show more when more recents to display, adjust handling for view more when .more BE response
703e1e9
to
3c9321a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @thanksameeelian! Thank you for adding in that last update for the max width
d70502b
into
learningequality:release-v0.16.x
Summary
the "Recent" section in
Learn
>Library
was initially displaying up to 12 recent items, and never displaying "View more" if there were 13+ items (due to a small, additional bug).this PR changes the "Recent" section to almost always initially display 3 cards (4 at a specific window size), rendering a "Show more" button if there are additional items. Once all 12 items in the initial BE response are shown, if there are still more recent items, a "View more" button will now render and allow the option to display those as well.
Before
User with 13 recent items - 12 shown with no option to see the 13th
(sidenav weirdness due to screenshot app)
After
Window larger than breakpoint 2
Window at breakpoint 2
Window smaller than breakpoint 2
User with 13 recent items, with "Show more" clicked
User with 13 recent items with "Show more" & "View more" clicked
References
Resolves #11438
Reviewer guidance
Learn
>Library
> "Recent" sectionTesting checklist
Critical user journeys are covered by Gherkin storiesPR process
If this is an important user-facing change, PR or related issue has a 'changelog' labelIf this includes an internal dependency change, a link to the diff is providedReviewer checklist
yarn
andpip
)