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

Topics page fixes #8928

Merged

Conversation

marcellamaki
Copy link
Member

Summary

  • Updates 'search' button to be 'filter' button per spec (and to reduce confusion about multiple references to 'search'ing)
  • Fixes tab behavior - tabs update main view and routes, whereas filter button opens filtering side panel
  • Folder tab doesn't display when we are at the last child folder
  • replaces breadcrumbs that seem to have disappeared on mobile

References

Fixes #8847

Figma spec for medium screens (where the toggling changes were most relevant)
Screen Shot 2021-12-14 at 3 48 00 PM

Implementation
updated-topics-page

updated-topics-mobile

Reviewer guidance

  • Any oversights or regressions that I missed?

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

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.

There are still some issues here from testing.

@rtibbles
Copy link
Member

When first navigating to the topics page, the folder header is not highlighted, because it requires the query parameter to be set.

Screenshot from 2021-12-15 05-50-33

Overall, I think the interaction on mobile is still confusing - the header links being used as buttons to toggle the side panels feels counterintuitive. As such, the fact that the button for filter is only available in tablet layout but not smaller layouts seems like an issue.

@rtibbles
Copy link
Member

Also, when first navigating to the search tab, I think we should show the search side panel - otherwise we are requiring users to click the search header and then open the search panel separately, making two clicks to express what they want to do.

…ow breadcrumbs on smaller screens, and only use tabs on desktop
@marcellamaki
Copy link
Member Author

Updated interactions -- this briefly broke my brain, but I think it covers the scenarios discussed earlier...
updated-topics-desktop

updated-topics-tablet

updated-topics-mobile

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.

Manual testing checks out - just need to prevent the redundant navigation error.

kolibri/plugins/learn/assets/src/views/TopicsPage.vue Outdated Show resolved Hide resolved
@rtibbles rtibbles merged commit 17c81df into learningequality:release-v0.15.x Dec 16, 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.

ux issues with 'search' tab in Library
2 participants