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

Remote content browsing final polish #10573

Merged

Conversation

akolson
Copy link
Member

@akolson akolson commented Apr 26, 2023

Summary

This pr polishes the remote content browsing UI

References

Resolves #10448

Reviewer guidance

UI should respond as specified in the mockups


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

@akolson akolson added the TODO: needs review Waiting for review label Apr 26, 2023
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: large labels Apr 26, 2023
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.

I've done a very quick code read through and nothing sticks out - and some of the vue component clean up looks 💯 - nice job @akolson

I also did some preliminary manual QA with pinned/ no pinned devices, as a guest user, and as a learner. So far looking good.

I feel okay about merging this and getting some good testing/feedback in during the bug catch tomorrow if @rtibbles doesn't see anything blocking in the code.

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.

We can merge this as is, but we should revisit the back links (as they will be broken on page refresh), and the delayed loading/loading state of the Explore libraries page.

Plus we should come back and fix the skipped tests too.

@@ -111,6 +111,7 @@ export default [
props: route => {
return {
deviceId: route.params.deviceId,
goBackRoute: route.params.goBackRoute,
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, this doesn't seem quite right to me. The goBackRoute prop of the LibraryPage is a route object, but route params are strings, so I don't quite see how this is meant to be working.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see how this is kind of working - the underlying params object does get modified, but it doesn't get encoded in the URL. Unfortunately, it means if the page is refreshed the back link no longer works.

I think this is probably best handled in a similar way to the other back links in Learn, by encoding the name of the page in the query parameters.

Can add an additional function to this composable for creating the library page back links: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/learn/assets/src/composables/useContentLink.js#L7

return accumulator;
}, []);

Promise.allSettled(fetchDevicesChannels).then(devicesChannels => {
Copy link
Member

Choose a reason for hiding this comment

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

There's no loading state in the page while this is happening, and because one of the devices was taking a very long time to respond, I was just looking at a blank Explore Libraries page with no indication what was happening.

I think this change to waiting until all the channel fetches have been resolved makes a worse user experience, because it means that seeing other libraries is limited by the loading speed of the slowest library to load.

@rtibbles rtibbles merged commit 4930a60 into learningequality:develop Apr 26, 2023
@akolson akolson deleted the remote-content-browsing-final-polish branch April 27, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: large TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Final Polish - Remote content browsing UI and ensuring full feature flow is to spec
3 participants