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

Quiz Creation Select Resources - Keyword search #11887

Merged

Conversation

AlexVelezLl
Copy link
Member

Summary

This PR adds a search input in Quiz Creation Select resources Side Panel.

Preview

Compartir.pantalla.-.2024-02-15.16_13_05.mp4

References

closes #11786

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: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels Feb 15, 2024
@@ -105,7 +105,10 @@
},
methods: {
handleClosePanel() {
if (this.workingResourcePool.length > this.activeResourcePool.length) {
if (
this.workingResourcePool &&
Copy link
Member Author

@AlexVelezLl AlexVelezLl Feb 15, 2024

Choose a reason for hiding this comment

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

For some reason there was a case where workingResourcePool was undefined here and it didn't let me move forward

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.

This looks great @AlexVelezLl - I'm going to list a few thoughts here but won't approve or request changes quite yet.

  • The changes I've made to the routing in Fix: Side panel closing confirmation & logic in child component #11862 fix most (if not all) of the bugs I'm seeing when testing this (ie, cannot save changes after adding search result, back button weirdness,
  • One thing that we didn't consider while working on this was setting the local loading state whenever we're fetching search results. When we run this locally it's so fast that it's hard to see how bad the UX would be when the load takes a few seconds. That said, however, I think that this is an issue we can better handle in a follow-up as this is not solely a problem w/ the search loading but with all of the loading in the side panel right now.

Great work - I'm going to wait for review on my latest changes to the PR I noted above and if that goes through, I think I'll merge this PR and then fix conflicts any conflicts it creates in my PR before merging that.

update - I've added a link to the project tech debt tracker. Updating how the loading states are managed will jive well with a general follow-up in how we manage the loading states altogether which I will work on after wrapping up question replacement early next week.

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 thing that looks suspect to me - conditionalizing setup exports will not let the component be reactive to changes in searchQuery state.

@@ -292,7 +342,7 @@
loading,
hasMore,
loadingMore,
fetchMoreQuizResources,
fetchMoreQuizResources: searchQuery ? fetchMoreSearchResults : fetchMoreQuizResources,
Copy link
Member

Choose a reason for hiding this comment

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

Unless the component is being torn down and recreated when the searchQuery updates (which I think we are trying to avoid happen), this value will not recompute.

You may want to instead make the fetchMoreQuizResources function behave conditionally on the searchQuery instead.

Also, it should be searchQuery.value I believe that we conditionalize on?

@AlexVelezLl
Copy link
Member Author

Thanks @rtibbles! I have updated it 👐

@rtibbles rtibbles dismissed their stale review February 24, 2024 01:30

Comments addressed

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.

Thanks for the updates and comments Alex & Richard.

@nucleogenesis nucleogenesis merged commit 78d136c into learningequality:develop Feb 26, 2024
31 checks passed
@AlexVelezLl AlexVelezLl deleted the eqm-keyword-search branch February 26, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quiz Creation Select Resources - Implement Keyword search.
3 participants