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.

Sign up for GitHub

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

Jump to bottom

My downloads page #10137

Merged
merged 17 commits into from
Mar 28, 2023
Merged

My downloads page #10137

merged 17 commits into from
Mar 28, 2023

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Feb 23, 2023 •
edited
Loading

Summary

New my downloads page.

Preview:

Screen.Recording.2023-03-17.at.07.25.45.mov

References

Specs in #9861

Reviewer guidance

  1. Go to menu
  2. Click on "My Downloads" menu item

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

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... SIZE: large labels Feb 23, 2023
rtibbles
rtibbles reviewed Feb 23, 2023
View reviewed changes
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.

I think for now, this can live as an additional single page app within the learn plugin (which will mostly just involve moving stuff from where you have it into Learn, and adding a sub-route for the my-downloads SPA to learn).

My reasoning for this is that while this is a separate single page app from Learn, with a separate side nav item, it is entirely dependent on learn, as that's the only place you can initiate downloads from.

...ri/plugins/my_downloads/assets/src/views/DownloadsList/PaginatedListContainerWithBackend.vue Outdated Show resolved Hide resolved
@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) SIZE: very large labels Mar 14, 2023
@AlexVelezLl AlexVelezLl marked this pull request as ready for review March 14, 2023 08:55
@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2023 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.16.0a9.dev0_git.38.g203fe02c.pex
Windows Installer (EXE) kolibri-0.16.0a9.dev0+git.38.g203fe02c-unsigned.exe
Debian Package kolibri_0.16.0a9.dev0+git.38.g203fe02c-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.0a9.dev0+git.38.g203fe02c-0.3.0.dmg
Source Tarball kolibri-0.16.0a9.dev0+git.38.g203fe02c.tar.gz
WHL file kolibri-0.16.0a9.dev0+git.38.g203fe02c-py2.py3-none-any.whl

AlexVelezLl
AlexVelezLl commented Mar 17, 2023
View reviewed changes
kolibri/plugins/learn/assets/src/composables/useContentLink.js
@@ -47,6 +47,29 @@ export default function useContentLink(store) {
return _makeLink(id, isResource, query);
}

function genExternalContentURLBackLinkCurrentPage(id) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Didnt find a better solution for getting the url of the content renderer to move from my_downloads to learn :/. Due to the fact that they are differents SPA they dont have the same router object

Copy link
Member

@rtibbles rtibbles Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems tractable, if a little complex. Another option here would be to create a content preview page within the downloads SPA, but this can work for now, especially as they both live in the same plugin, so cannot be deactivated separately.

It's possible that some of the work that @marcellamaki is doing in her app bar PR could be reused here to simplify this slightly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we may be able to repurpose that here, and once this is merged, I can also do a follow up to change the side navigation item to match the new CoreMenuOption API @rtibbles

@AlexVelezLl AlexVelezLl requested a review from rtibbles March 17, 2023 12:41
@AlexVelezLl AlexVelezLl added the TODO: needs review Waiting for review label Mar 17, 2023
@MisRob MisRob mentioned this pull request Mar 21, 2023
Closed
1 task
marcellamaki
marcellamaki approved these changes Mar 28, 2023
View reviewed changes
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.

Looks good @AlexVelezLl ! Nice work.

I think this piece is ready to merge. Can you create a follow up issue that tracks the additional pieces that are remaining to get this working, once the API is ready, and not mocked? (code cleanup of dummy data, API, filtering using the API endpoint for the paginated list containers, etc.) Let me know if you have questions and we can go over it together.

kolibri/plugins/learn/assets/src/composables/useContentLink.js
@@ -47,6 +47,29 @@ export default function useContentLink(store) {
return _makeLink(id, isResource, query);
}

function genExternalContentURLBackLinkCurrentPage(id) {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we may be able to repurpose that here, and once this is merged, I can also do a follow up to change the side navigation item to match the new CoreMenuOption API @rtibbles

@marcellamaki marcellamaki merged commit 22e1236 into learningequality:develop Mar 28, 2023
This was referenced Apr 4, 2023
Closed
Closed
@jredrejo jredrejo mentioned this pull request Apr 6, 2023
Merged
9 tasks
@rtibbles rtibbles mentioned this pull request Apr 11, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcellamaki marcellamaki marcellamaki approved these changes

@rtibbles rtibbles Awaiting requested review from rtibbles

Assignees
No one assigned
Labels
APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... SIZE: large SIZE: very large TODO: needs review Waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AlexVelezLl @rtibbles @marcellamaki