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

Revert change that prevented search redirect in all cases. #12236

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 4, 2024

Summary

  • Added another condition for TOPICS_TOPIC_SEARCH redirection #12019 introduced a regression that caused the search redirect never to occur for folders with only leaf nodes as children
  • Reverts most of the changes in that PR to restore former behaviour
  • Retains the setting of sidePanelOpen to false, but just sets this whenever topic navigation occurs

References

Bug introduced:
Screenshot from 2024-06-04 08-34-33

Reviewer guidance

Navigate to a folder with only leaf nodes as children on desktop.
Confirm that the search side nav properly displays.
Switch to mobile browsing in dev tools.
Navigate down the topic hierarchy using the folders sidepanel.
Ensure that each time you navigate, the side panel is properly hidden once you have navigated to the next folder.


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

Fix bug by always closing the sidepanel on navigation.
@rtibbles rtibbles requested review from marcellamaki and pcenov June 4, 2024 16:20
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend labels Jun 4, 2024
@rtibbles rtibbles added the TODO: needs review Waiting for review label Jun 4, 2024
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.

Reverted code changes look correct here, as well as the setting sidePanelIsOpen to false.

will wait for Peter's confirmation though :)

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@rtibbles rtibbles merged commit cb1fcd7 into learningequality:develop Jun 8, 2024
31 checks passed
@rtibbles rtibbles deleted the lets_get_topical branch June 8, 2024 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants