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

Update Topics page header navigation #8894

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

marcellamaki
Copy link
Member

Summary

  • Updates tabs for consistency, and updates routes to support <HeaderTabs /> usage
  • Tabs fix shifting hover state
  • Ensures no thumbnail <> tab overlay on mobile (desktop already fixed)

References

Fixes #8630
tabs-toggle

Reviewer guidance

  • Any problems/unhandled edge cases with the routing/redirect 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

@marcellamaki marcellamaki added the TODO: needs review Waiting for review label Dec 10, 2021
Comment on lines +349 to +350
import HeaderTabs from '../../../../coach/assets/src/views/common/HeaderTabs';
import HeaderTab from '../../../../coach/assets/src/views/common/HeaderTabs/HeaderTab';
Copy link
Contributor

@indirectlylit indirectlylit Dec 10, 2021

Choose a reason for hiding this comment

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

Glad this was a straight-forward drop-in replacement!

Preferably we'd move this to core API spec (to avoid cross-plugin references) or even KDS (to make this component fully documented and reusable across products)

I'll let you and @rtibbles decide whether that should happen now or later

Copy link
Member

@rtibbles rtibbles Dec 14, 2021

Choose a reason for hiding this comment

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

I'm going to vote for later. Thanks to our updated string machinery, cross plugin imports like this are less of a concern, but the long import paths are not great.

Also good to think about the contrast between code used in common between apps vs things we want to expose as a common API.

I think most of our core API spec is the former at the moment, whereas it should be exclusively the latter.

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.

This is a big improvement, nice job!

The only thing I'm noticing (aside from #8627) is that the header tabs behave a little strangely. At smaller sizes the 'Search' tab launches the side panel which seems incorrect:

2021-12-13 21 44 28

The tabs also initially start out with neither selected:

2021-12-13 21 41 16

I'm happy to open a follow-up issue for this if you'd like

@rtibbles
Copy link
Member

I think there's a related issue here: #8847

@rtibbles rtibbles merged commit c7285de into learningequality:release-v0.15.x Dec 14, 2021
@indirectlylit
Copy link
Contributor

updated that issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issues with library top-nav
3 participants