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

Improve learn folder contents display #12737

Conversation

rtibbles
Copy link
Member

Summary

  • Reverts the folder side panel to show the topic level contents of the folder, rather than the skip logic folders
  • Adds a visual divider between the end of the contents of a subtopic, and resources at the top level of the current topic
  • Rejigs content display so that topics and resources are now shown in the proper order in the topic, with each subtopic displayed in-line, and then contiguous sequences of resources displayed together

References

Fixes #9043
Fixes #10060

Reviewer guidance

Look at large topics, small topics, deeply nested topics, topics that have resources and topics interspersed.

Good channels to look at: Kolibri QA channel, the HTML5 topic, Ciencia NASA channel - the Ciencia Espacial
topic.

Folder side panel:

Before After
Screenshot from 2024-10-22 15-48-20 Screenshot from 2024-10-22 15-48-08

Resources in a topic after a subtopic now have a divider:

Screenshot from 2024-10-22 16-00-19


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

  • 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 TODO: needs review Waiting for review APP: Learn Re: Learn App (content, quizzes, lessons, etc.) labels Oct 22, 2024
@rtibbles rtibbles force-pushed the folders_folders_everywhere branch from 2762886 to de8778e Compare October 22, 2024 23:44
@pcenov pcenov self-requested a review October 23, 2024 15:18
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.

Hi @rtibbles I confirm that this is implemented as specified, no issues observed while manually testing!

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.

At this point I only have questions, I'm not requesting specific changes, although depending on teh answers and how much I'm understanding (or not), I might have some suggestions about tweaks/additions to the code comments for some of the logic.

Prevent display of top level topic resources in subtopic display.
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.

With QA team approval and questions answered, this is good to merge. I do think it's a little bit dense with an array of either an array or objects, but I also think the solution is a good approach. ✅

@marcellamaki marcellamaki merged commit a3ec15b into learningequality:release-v0.17.x Oct 25, 2024
34 checks passed
@rtibbles rtibbles deleted the folders_folders_everywhere branch October 25, 2024 18:40
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.) DEV: frontend TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants