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

Fix show more of top level resources #9555

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Jul 8, 2022

Summary

Updates the "resource" display logic to be more distinct from topics and search results logic and to have separate management of display.

References

Fixes #9553

Reviewer guidance

Not-logged in and logged-in views:

not-logged-in

Jul-08-2022.08-44-48.mp4

QUESTIONS

  1. Is this too "simple" of a fix, i.e. should it be more complex to potentially allow for more incremented "view more" display? (such as appending 10 more at a time, rather than just all?)

  2. Is this too different from some of the composable setup?

  3. Have I accidentally broken anything else?

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

  • Automated test coverage is satisfactory
  • 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

@marcellamaki marcellamaki requested a review from rtibbles July 8, 2022 12:47
@marcellamaki marcellamaki requested a review from rtibbles July 8, 2022 19:21
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

One other regression to fix here.

kolibri/plugins/learn/assets/src/views/TopicsPage.vue Outdated Show resolved Hide resolved
@marcellamaki marcellamaki requested a review from rtibbles July 11, 2022 18:21
@rtibbles rtibbles dismissed their stale review July 11, 2022 18:27

All comments resolved. This should be good to merge pending manual QA.

@marcellamaki marcellamaki requested a review from radinamatic July 11, 2022 18:33
@radinamatic
Copy link
Member

Is this too "simple" of a fix, i.e. should it be more complex to potentially allow for more incremented "view more" display? (such as appending 10 more at a time, rather than just all?)

Ideally yes, but might not be a blocker at this point.

Have I accidentally broken anything else?

Browsed this and other channels, nothing jumped out at me 🙂

Signed-in learner (Firefox) and guest (Chrome)

Ubuntu18 04 (start)  Running  - Oracle VM VirtualBox_002

Signed-in admin (Firefox) and guest (Chrome)

Ubuntu18 04 (start)  Running  - Oracle VM VirtualBox_003

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

LGTM! 💯 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants