debounced updateWindow

[Jaspreet-singh-1032]

fixes #461

I also linked this with Kolibri and tested this out, and It seems to be working fine for me.

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change has been added to the changelog

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

Comments

[indirectlylit]

Just FYI, requestAnimationFrame might be appropriate here because it implements a dynamic throttle. The old responsive window mixin used the frame-throttle wrapper library for this:

kolibri-design-system/lib/KResponsiveWindowMixin.js

Line 59 in e578ede

import { throttle } from 'frame-throttle';

kolibri-design-system/lib/KResponsiveWindowMixin.js

Lines 74 to 77 in e578ede

	const windowResizeHandler = throttle(() => {
	const metrics = windowMetrics();
	windowListeners.forEach(cb => cb(metrics));
	});

[Jaspreet-singh-1032]

Hello @indirectlylit, Thanks for your review. Just to confirm, do I need to implement it similar to how it is implemented in KResponsiveWindowMixin.js using throttle?

[indirectlylit]

Hi @Jaspreet-singh-1032 - that's for you (and @MisRob) to decide. This is a very minor note and should not block the PR.

That said if you're interested, some links:

• https://css-tricks.com/using-requestanimationframe/
• https://css-tricks.com/debouncing-throttling-explained-examples/

Personally, I prefer using this form of throttling for anything related to fluid UI changes because the system can choose an appropriate refresh rate rather than hard-coding a somewhat arbitrary delay period.

[Jaspreet-singh-1032]

Thank you, @indirectlylit, I was previously unaware of this function. I will read about it and If it is a better option then I will definitely use it here.

[rtibbles]

For most browsers this will use requestAnimationFrame, which will invoke the callback just before the next repaint - which means that it should not be the cause of a reflow by reading the inner width and height of the window (which is a strong part of the motivation for this issue), so I do think this is a good change to use here.

[Jaspreet-singh-1032]

Thanks, @rtibbles, I read some blogs about requestAnimationFrame and I also think this is a better option to use here.

[MisRob]

Thanks @indirectlylit, that's a good idea

[akolson]

Hi @Jaspreet-singh-1032, with exception build errors that I think are resolvable(Some linting issues at the very least), the changes made, make sense me. (cc @MisRob on the build failures )

[MisRob]

I'll push update that will resolve the changelog check after all remaining checks are working and the PR is approved (@akolson this will be done by reviewers eventually for majority of PRs from our contributors, I will follow-up soon)

[MisRob]

@akolson It's not very clear to me if your comment is meant to be approval? Could you please mark it if that's the case? I will then complete the changelog.

[akolson]

Hi @MisRob! My comment was meant to specifically address the build failure. I see these have been addressed(or to be addressed). I am more confident approving it now. Thanks

[akolson]

LGTM!

[MisRob]

Okay @akolson, thank you. Changelog is updated.

[MisRob]

Checks should be okay now. Would recommend Kolibri QA pass as part of review. It's up to you how you want to proceed.

[MisRob]

Seems all is done here so merging

[MisRob]

@Jaspreet-singh-1032 One of my colleagues noticed an issue with using setTimeout after we merged it, so I opened a follow-up #480. You don't need to work on it, of course! It's rather FYI in case you'd find the note interesting. I didn't realize that either.