Match media testing guard and bug fix #470

rtibbles · 2023-10-12T19:58:55Z

Description

Adds an additional guard to prevent attempting to access the window.matchMedia API when it is in an environment that it is not supported
Fixes a hitherto unnoticed bug, whereby the use of a JS getter to return the value of window.matchMedia meant that removing event listeners was doing nothing, as the getter was returning a new instance each time (and so was not unbinding the listener from the instance that it had actually been bound to).
Does this by caching the window.matchMedia result call, to minimize code changes and to continue to allow delay of instantiation of the mediaQueryList object.

Issue addressed

Fixes failing tests seen in develop on Kolibri
Fixes unreported bug that would stop listeners from being unbound

Testing checklist

The change has been added to the changelog

MisRob · 2023-10-13T06:43:45Z

CHANGELOG.md

@@ -47,6 +47,17 @@ Changelog is rather internal in nature. See release notes for the public overvie

 ## Version 2.0.0

+- [#470]


Could you please move this istem to the "Upcoming version"?

The current items in the "Version 2.0.0" are already released. I will rename this section to "Version 2.0.0-beta0 (released)" to make it clearer. If you have some others ideas of what may help, let me know.

For now I updated the changelog in this way

From my experience looking through dependency updates for many different JS packages - I have found the style of having changes only mentioned in alphas, betas, and rcs and not in the final release much more difficult to parse, especially when looking for breaking changes. I'd recommend not separating the changelog in this way, and see the beta releases as try outs for what the final release will be.

That makes sense and will be ideal in the long term.

This is temporary only to help us now when we're in the transition towards re-introducing a stable release process. We already have beta with release notes so that we can start introducing and regression testing. Without them, it'd be difficult because we were postponing releases and accumulated lots of breaking changes. So release notes were created in advance and with that we will need to keep track for some time what other changes need to be eventually added.

This should get better with more frequent releases. With @marcellamaki and @thanksameeelian, we decided to cleanup the release notes from all betas and merge them for the final release as soon as it's ready so that should address it. On that occasion we can do one last changelog cleanup and merge all sections for v2.

cc @thanksameeelian @marcellamaki

Alternatively, we could just add a comment to the changelog to indicate where the release notes end. I tried something like that already, but it seems it's hard to notice for devs. In any case, I'd be okay with any (shared) way of making it easier for us to track where the existing release notes end in the changelog, if you have any other ides.

Will rebase now to move this.

akolson

Code additions look correct to me. Thanks @rtibbles

rtibbles · 2023-10-17T16:45:03Z

Updated!

MisRob · 2023-10-19T10:46:50Z

I wanted to note that I only jumped in to notify about changelog, but didn't review myself. If there is anything needed from my side, let me know.

so that we do actually unbind our previously bound listeners.

rtibbles · 2023-10-19T14:54:58Z

Rebased again to resolve the CHANGELOG merge conflict.

At the risk of sounding needy, I do need approval - the changes are fairly straight forward so would be a quick review.

MisRob · 2023-10-19T15:55:22Z

I assume that @akolson review was meant as approval so marking it green. Please reach out if anything @akolson

MisRob · 2023-10-19T16:17:27Z

I skimmed it too now and haven't noticed anything blocking. Leaving only one minor note.

lib/useKResponsiveWindow/MediaQuery.js

MisRob · 2023-10-23T11:27:02Z

CHANGELOG.md

+  - **Description:** Fix bug and add test guard in MediaQuery implementation
+  - **Products impact:** none
+  - **Addresses:** -
+  - **Components:** none


Just a note that here, all affected components would ideally be included here even though it doesn't modify them directly, it's something that's used under the hood (this will help with the release notes).

No need to do anything about it, I already prepared the release notes for this item too. Just jumping into PRs and using them as spreading the word.

MisRob · 2023-10-23T11:35:37Z

CHANGELOG.md

@@ -6,6 +6,17 @@ Changelog is rather internal in nature. See release notes for the public overvie

 <!-- All new changelog items should come here -->

+- [#470]
+  - **Description:** Fix bug and add test guard in MediaQuery implementation
+  - **Products impact:** none


Here, the product impact would be bugfix. Here's an example of release notes for useKResponsiveWindow. We'd ideally be able to read this from the changelog rather than opening a PR link and studying it. As mentioned below, no action needed.

rtibbles requested review from akolson and thanksameeelian October 12, 2023 19:58

MisRob reviewed Oct 13, 2023

View reviewed changes

akolson reviewed Oct 16, 2023

View reviewed changes

rtibbles force-pushed the no_match_media branch from d746c53 to 9814654 Compare October 17, 2023 16:44

rtibbles added 3 commits October 19, 2023 07:51

Add a guard to prevent access to window.matchMedia when it is undefined.

a8f0177

Persist the mediaQueryList instance generated by window.matchMedia

0bbd25d

so that we do actually unbind our previously bound listeners.

Update changelog post hoc to have PR to point to.

75e1515

rtibbles force-pushed the no_match_media branch from 9814654 to 75e1515 Compare October 19, 2023 14:54

MisRob approved these changes Oct 19, 2023

View reviewed changes

MisRob reviewed Oct 19, 2023

View reviewed changes

lib/useKResponsiveWindow/MediaQuery.js Show resolved Hide resolved

rtibbles merged commit 0444139 into learningequality:main Oct 20, 2023
7 checks passed

rtibbles deleted the no_match_media branch October 20, 2023 16:08

MisRob reviewed Oct 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match media testing guard and bug fix #470

Match media testing guard and bug fix #470

rtibbles commented Oct 12, 2023 •

edited

Loading

MisRob Oct 13, 2023

MisRob Oct 13, 2023

rtibbles Oct 15, 2023

MisRob Oct 16, 2023

MisRob Oct 16, 2023 •

edited

Loading

rtibbles Oct 17, 2023

MisRob Oct 19, 2023

akolson left a comment

rtibbles commented Oct 17, 2023

MisRob commented Oct 19, 2023

rtibbles commented Oct 19, 2023

MisRob commented Oct 19, 2023

MisRob commented Oct 19, 2023

MisRob Oct 23, 2023

MisRob Oct 23, 2023

MisRob Oct 23, 2023 •

edited

Loading

		@@ -47,6 +47,17 @@ Changelog is rather internal in nature. See release notes for the public overvie

		## Version 2.0.0

		- [#470]

Match media testing guard and bug fix #470

Match media testing guard and bug fix #470

Conversation

rtibbles commented Oct 12, 2023 • edited Loading

Description

Issue addressed

Testing checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

rtibbles commented Oct 17, 2023

MisRob commented Oct 19, 2023

rtibbles commented Oct 19, 2023

MisRob commented Oct 19, 2023

MisRob commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

rtibbles commented Oct 12, 2023 •

edited

Loading

MisRob Oct 16, 2023 •

edited

Loading

MisRob Oct 23, 2023 •

edited

Loading