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

Refactor sidebar to use slots #10903

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

marcellamaki
Copy link
Member

Summary

Makes use of slots
Reduces the number of sticky calculations (no longer one for the tabs)

References

sticky-scroll.mp4

Reviewer guidance

This whole approach might be questionable because it is Friday evening


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 requested a review from rtibbles June 23, 2023 22:44
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Jun 23, 2023
@thanksameeelian
Copy link
Contributor

this is not my actual sentiment, but reading your branch name made my brain go right to:

stop-side-barring.mp4

@thanksameeelian
Copy link
Contributor

thanksameeelian commented Jun 23, 2023

i noticed that this introduced a bit of weirdness with the sidebar headers that i couldn't recreate on develop - the "search" header stretching beyond the bounds of the sidebar it's heading up when the window is medium size.
Screen Shot 2023-06-23 at 6 04 04 PM

sidebar-headers.mov

i didn't encounter the same thing on develop:
Screen Shot 2023-06-23 at 6 10 20 PM

even though it's a small difference, i think it takes away a little bit from the clarity of what those headers control.

as an aside, i think it would be really nice if we could use CSS to truncate the search box placeholder text & add an ellipsis in this medium-size case where the text is not fully visible, as it cuts off a bit abruptly and as placeholder text isn't scrollable, so it feels a bit mysterious if you're only viewing it in a medium window size haha. but i know it's the same way on develop! it just caught my eye during this review and i thought it might be worth a small follow-up issue if others agree.

@marcellamaki
Copy link
Member Author

marcellamaki commented Jun 23, 2023

the "search" header stretching beyond the bounds of the sidebar it's heading up when the window is medium size

ah good catch, I thought I had reverted that (unintentional) change! yes this should definitely be fixed

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.

Merging to prevent the sidebar issue interfering with QA.

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.) SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants