Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Corrected output for filtered Quizzes and Lessons #11313

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

radinamatic
Copy link
Member

@radinamatic radinamatic commented Sep 28, 2023

Summary

'No results' string for filtered Quizzes displays properly:

string-fix-quizzes.mp4

There is probably a more elegant way of fixing the second reported issue, but seems to be working OK now 🙂
On top of the 'No results' not being displayed for not visible lessons, it was displaying for visible ones even when there are no lessons at all, below the 'You do not have any lessons' message.

Before After
https://github.com/learningequality/kolibri/assets/1457929/32cede85-7bd0-4e47-b40e-ab97da45cac1 https://github.com/learningequality/kolibri/assets/1457929/bcdbb015-f039-482a-9053-5eb9e744760b

There may still be an underlying loading issue (brief flash of 'No results' until the lesson is loaded and appears in the table), but is probably out of the scope of this PR and definitely above my pay grade 😅

References

Fixes #11310

Reviewer guidance

Change the status of the quizzes (start/end) and lessons (visible/not visible), and check if the 'No results' message is displayed properly for all filtered cases.


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

@radinamatic radinamatic added TODO: needs review Waiting for review APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) labels Sep 28, 2023
@pcenov
Copy link
Member

pcenov commented Sep 28, 2023

LGTM - let's further discuss and report separately any additional improvements which can be made such as the brief flash of 'No results' and the correct position of the Status filter :)!

Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Kudos @radinamatic :)! Changes are looking good to me.

This is not blocking, just a suggestion how we could tweak naming and organization of conditions to improve readability. It took me some time to make all the connections due to unclear naming, the number of negations and having conditions at various places. This was something present before your update though - it's up to you if you'd like make these changes.

Template:

<p v-if="showNoResultsLabel">{{ coreString('noResultsLabel') }}</p>

JavaScript:

  hasVisibleLessons() {
    return this.activeLessonCounts.true
  },
  hasNonVisibleLessons() {
    return this.activeLessonCounts.false
  },
  showNoResultsLabel() {
    if (!this.lessons.length) {
      return false
    }
    if (this.filterSelection.value === 'filterLessonVisible') {
      return !this.hasVisibleLessons
    }
    if (this.filterSelection.value === 'filterLessonNotVisible') {
      return !this.hasNonVisibleLessons
    }
  },

@radinamatic
Copy link
Member Author

Thank you @MisRob for suggestions on naming improvements (and @marcellamaki for last mile support), all done! 🙏🏽

@rtibbles rtibbles merged commit 48b414d into learningequality:release-v0.16.x Sep 29, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coach>Plan - Missing 'No results' string (CommonCoreStrings.noResults)
4 participants