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.

Sign up for GitHub

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

debounced updateWindow #469

Merged
merged 6 commits into from
Oct 19, 2023
Merged

debounced updateWindow #469

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d425bd5
debounced updateWindow
Jaspreet-singh-1032 Oct 12, 2023
5a569c6
use throttle to optimize updateWindow
Jaspreet-singh-1032 Oct 14, 2023
9f4785b
fix lint issue
Jaspreet-singh-1032 Oct 16, 2023
a5e325f
Merge branch 'main' into issue-461-optimize-useKWindowDimensions-perf…
MisRob Oct 17, 2023
17c0e6e
Update changelog
MisRob Oct 17, 2023
cf2909e
Merge branch 'main' into issue-461-optimize-useKWindowDimensions-perf…
MisRob Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/useKWindowDimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import './composition-api'; //Due to @vue/composition-api shortcomings, add plugin prior to use in kolibri, studio and tests
import { onMounted, onUnmounted, ref } from '@vue/composition-api';
import debounce from 'lodash/debounce';
Copy link
Contributor

Choose a reason for hiding this comment

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

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));
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @indirectlylit, that's a good idea


/** Global variables */
export const windowWidth = ref(null);
Expand All @@ -16,6 +17,8 @@ const updateWindow = () => {
windowHeight.value = metrics.height;
};

const debouncedUpdateWindow = debounce(updateWindow, 200);

/**
* Returns window inner width and height
* @returns {Object} The window's inner width and height
Expand Down Expand Up @@ -73,12 +76,12 @@ function isUseKWindowDimensionsActiveElsewhere() {
export default function useKWindowDimensions() {
onMounted(() => {
usageCount.value++;
addWindowListener(updateWindow);
addWindowListener(debouncedUpdateWindow);
});

onUnmounted(() => {
usageCount.value--;
removeWindowListener(updateWindow);
removeWindowListener(debouncedUpdateWindow);
});

return {
Expand Down
Loading