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

updated KDS reference & testing after changes to useKResponsiveWindow #11315

Conversation

thanksameeelian
Copy link
Contributor

@thanksameeelian thanksameeelian commented Sep 28, 2023

Summary

this links Kolibri to KDS PR 453 - "useKResponsiveWindow update fixing resizing behavior in Learn > Library" which fixes #11212 and potentially other pages where windowBreakpoint miscalculations were occurring under the hood.

the KDS PR fixes a bug observed during QA review in Learn > Library - in certain cases, rapidly resizing the browser window would cause the sidepanel to automatically display when the screen was small, behavior we did not expect nor want at that screen size. beneath the surface, the components were receiving conflicting information about the current screen size because useKResponsiveWindow was not updating in the way we expected.

to address those issues, the KDS PR introduces changes to the scope of variables within useKResponsiveWindow and to the way the composable uses useKWindowDimensions.

References

closes #11212

before

2023-09-08_17-03-37.mp4

after

resizing-successfully.mov

Reviewer guidance

  • with at least one channel imported, visit Learn > Library
  • quickly resize the window dimensions (see "before" video to replicate exact behavior that once triggered the bug)
  • when window is resized to be smaller than 601px, side panel should not automatically display - responsive elements & layout should resize & adjust as expected

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included

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

@thanksameeelian thanksameeelian force-pushed the side-panel-opening-after-resize branch from 9121973 to 0826964 Compare September 29, 2023 16:04
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Sep 29, 2023
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Tested this and worked perfectly

@thanksameeelian thanksameeelian force-pushed the side-panel-opening-after-resize branch from 0826964 to 1022ed6 Compare October 3, 2023 22:09
@thanksameeelian thanksameeelian changed the title [DO NOT MERGE] Temporary KDS reference for testing updates to useKResponsiveWindow updated KDS reference & testing after changes to useKResponsiveWindow Oct 3, 2023
return mount(LearningActivityBar, { propsData });
// stubbing out KCircularLoader, as using the actual component led to errors related to
// Vue Composition API - stub may not be needed once we upgrade to Vue 2.7
return mount(LearningActivityBar, { propsData, stubs: ['KCircularLoader'] });
Copy link
Member

Choose a reason for hiding this comment

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

Thansk @thanksameeelian for cleaning this up, I think that it got broken as result of my recent circular loader updates.

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.

All good, thank you

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 SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library page - The filter defaults to being open
3 participants