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

Update client width to use getBoundingClientRect #573

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Mar 12, 2024

Description

This PR replaces the use of clientWidth to use getBoundingClientRect().width to have a more accurate measurement of the width of the KListWithOverflow items. It also recalculates the width of the moreButton element on each overflow calculation to make sure this value hasnt changed.

Before/after screenshots

In very specific cases, using clientWith can lead to cases like this due to the lack of decimals:

image

Now this is fixed:

image

Changelog

  • #573
    • Description: More precise calculation of list with in KListWithOverflow.
    • Products impact: bugfix.
    • Addresses: -.
    • Components: KListWithOverflow.
    • Breaking: no.
    • Impacts a11y: no.
    • Guidance: -.

Testing checklist

  • Contributor has fully tested the PR manually
  • [ x If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

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

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

@AlexVelezLl AlexVelezLl changed the base branch from develop to release-v3 March 12, 2024 14:52
@AlexVelezLl AlexVelezLl requested a review from MisRob March 12, 2024 14:52
if (!element) {
return 0;
}
return element.getBoundingClientRect().width;
Copy link
Member

Choose a reason for hiding this comment

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

One thing to flag, getBoundingClientRect forces a reflow, so this can cause performance issues. More info here: https://toruskit.com/blog/how-to-get-element-bounds-without-reflow/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Richard, I will batch all getWidth calls before changing anything in the DOM

@AlexVelezLl
Copy link
Member Author

Hi @rtibbles I have batched all calls to getBoundingClientRect before setting the items visibilities. Hope this helps to optimize the performance and avoids reflows.

@rtibbles
Copy link
Member

It will still be quite a few, as we are calling it for each element in the list, it seems?

I wonder if it might be better to take the Intersection Observer approach here, and require that Kolibri integrate the Intersection Observer polyfill for unsupported browsers.

I think we can merge this as is, but maybe we file a follow up issue for a potential performance improvement.

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented Mar 12, 2024

Oh I had read here that if there is no changes to the DOM between calls to properties that needs the layout to be recalculated, then the layout is no recalculated as is already up to date. What causes reflow is when we invalidate the layout by changing the DOM nodes, so I think batching all these calls at the beginning would do the work.

This is a similar approach they do in their example with multiple different DOM nodes in a list.

@rtibbles
Copy link
Member

Oh, great - thanks for doing the due dilligence here! No need for the follow up, which would add some complexity.

@MisRob MisRob removed their request for review March 21, 2024 08:02
@rtibbles rtibbles merged commit a07fcde into learningequality:release-v3 Mar 26, 2024
8 checks passed
@AlexVelezLl AlexVelezLl deleted the klistwithoverflow-fix-decimals branch March 26, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants