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

Bookmarks feature #8272

Merged
merged 12 commits into from
Sep 14, 2021
Merged

Conversation

Evgeni998
Copy link
Contributor

@Evgeni998 Evgeni998 commented Aug 11, 2021

Summary

Add a new functionality to see your bookmarks in the creation of new quiz menu (Coach -> Plan ->Quizzes -> New Quiz -> Create New Quiz) and also in the Manage lessons menu (Coach -> Plan -> Lessons -> Lesson -> Manage Resources). The functionality works fine when you open the bookmarks and if you try to go deeper into nested folders, but the ResourceSelectionBreadcrumbs component hasn't been implemented to work yet (in other words, you can't navigate into the different stages of the folder chain yet, I will attach a screenshot, to show you what I mean.) The bookmarks that are seen are dynamic (come from the API), and if you add a new bookmark it should show in the bookmark component.
NOTE: I have left the old route watcher logic in the LessonResourceSelection page because the logic is the same as in the CreateExamPage file, and I am using it as a template of how to make it work properly and change it later there as well
Screenshot from 2021-08-13 13-32-57

Here are alson screenshots of the bookmark menu as it is right now

Screenshot from 2021-08-13 13-40-29
-----------------------------------------------------------------------------------------------------------
Screenshot from 2021-08-13 13-41-04

References

Here are the issues that the PR is trying to close
#8210
#8211

Reviewer guidance

You can test the changes by going to Coach -> Plan ->Quizzes -> New Quiz -> Create New Quiz and Coach -> Plan -> Lessons -> Lesson -> Manage Resources. You can also manually make a POST request to create a new bookmark and see it reflect on the current bookmark component.


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

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

Hi @Evgeni998 thank you for your work so far on this! A few things caught my eye so I'm just leaving some quick notes inline.

Also, it's always appreciated to fill in the PR description template, especially with links to any mockups or specs you're following. That makes reviewing much easier.

thanks again!

.bash_profile Outdated Show resolved Hide resolved
kolibri/core/assets/src/views/ContentIcon.vue Outdated Show resolved Hide resolved
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.

Nice work so far! I've added a few follow ups to Devon's comments and I'll set up a time that we can talk through the feedback, what you're still working on, and what you may have questions about.

@indirectlylit
Copy link
Contributor

@Evgeni998 FYI once you've addressed some feedback from the PR, feel free to hit the 'resolve' button:

image

thanks!

@marcellamaki
Copy link
Member

Hi @Evgeni998 - you've made a lot of great updates on this, so thanks for your revisions. Overall, most of the functionality seems to be working on the front end and I think this is very close! I'm going to take a closer look at the code itself and provide some feedback and ask a few team members to take a look also, but the one thing I noticed with a quick manual QA is that single listed bookmarks on the don't seem to be added on check, whereas if you navigate into a bookmarked topic, the functionality is working as expected. Just wanted to raise this so you could take a look as folks continue to review today, so you have something to keep working on. Please feel free to reply in the comments if you have any questions.

I've included a gif below for reference - you will see the first resource that I check, there is no snackbar indicator, and after closing the modal it doesn't show up on the list of resources, whereas the second selection, after navigating into a topic, works well!

bookmarks-single-resouce

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Great work so far! I spotted a few places where we might run into making too many DB queries and posted a few questions.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I left a couple more comments requesting some additional refactors. I would be happy to chat further next week.

A couple other notes:

1 - Overall, I'm happy to get this PR merged sooner than later and we can start tying up loose ends - one of which is that it will probably be best to paginate how we get the Bookmarks and may need some design guidance there (@jtamiace ) - all of this could be tackled in a follow up issue.

2 - This is one issue that is probably going to be a bit complex to work out, but currently the routing is a bit wonky when you start going into your Bookmarks.

  • When viewing Bookmarks in Lesson creation, the back button goes to a broken version of the previous page.
  • One cannot navigate back and forth between "Lesson Creation -> Bookmarks -> Bookmarked Topic -> Subtopics ..."
  • I think Bookmarks should have its own entire page component rather than piggy-backing on LessonResourceSelectionPage - this will probably make routing within the Bookmarks easier to accomplish too as we may want to introduce the pagination mentioned above as well.

@jtamiace - I'm not sure how much of - if any of the above should be applied in this PR vs a follow-up discussion and/or issue. It seems kind of ambiguous from the user's perspective where they are between searching for content and viewing their bookmarks as is.

@jtamiace
Copy link
Member

One cannot navigate back and forth between "Lesson Creation -> Bookmarks -> Bookmarked Topic -> Subtopics ..."

@nucleogenesis follow up might be best for this because looking at the designs it's not really clear what the intended behavior should be when you click into a subtopic from bookmarks. Does the current implementation take the user out of the bookmarks and into "Channel > Topic > Subtopic"?

And for pagination, I've used this in the topic/category search UI that can also be used for bookmarks. I'm not exactly sure how many results per "page" would be best for performance though.

Screen Shot 2021-08-30 at 10 58 05 AM

…sonContentCard> component and optimize data fetching logic
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.

Per meeting between @Evgeni998 and @nucleogenesis and follow up conversations I've had with @nucleogenesis and @rtibbles, going to go ahead and merge this, and @nucleogenesis will create follow up issues as needed.

@marcellamaki marcellamaki dismissed nucleogenesis’s stale review September 13, 2021 20:10

Per follow up conversation on slack, changes will be handled in follow up issues

@marcellamaki marcellamaki merged commit 2d98aaa into learningequality:develop Sep 14, 2021
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.

6 participants