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

Don't use KResponsiveElementMixin in all ContentCards #8123

Merged

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented May 28, 2021

Summary

Remove a piece of code that pushes KResponsiveElementMixin into ContentCard in <script> part above export default
of ContentCardGroupCarousel.

Due to how files are bundled, the removed piece of code is evaluated on the application start, no matter whether ContentCardGroupCarousel is mounted or no. Consequently, KResponsiveElementMixin is unintentionally used in all ContentCards in the whole app, not only in ContentCardGroupCarousel itself. This means unnecessary performance losses whenever ContentCard is used on a page because KResponsiveElementMixins ResizeSensor causes render tree layouts/reflows.

Regarding the intended usage for ContentCards that are children of ContentCardGroupCarousel, elementWidth and elementHeight provided by KResponsiveElementMixin are not used in ContentCard at all, and I haven't noticed any problems with the carousel during testing after removing this code. Removed logic, including the warning in a comment, is quite old. I assume that it might be obsolete.

The carousel recording for various screen sizes taken after the update:

carousel

References

Related #7525

Reviewer guidance

Please check possible regressions of ContentCardGroupCarousel.

Currently, it is used in the following places:

  1. Learn -> Recommended -> there are three carousels, one for each of the following sections: Most Popular, Next Steps, Resume

carousel-recommended

  1. Learn -> Channels -> Open a resource -> there are two carousels, one for each of the following sections: Next resource, Recommended

carousel-resource

Depending on your test data, it's possible that not all carousels will be visible. I took screenshots above after tweaking the code a bit to display carousels all the time so I could test properly all places.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

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

Remove a piece of code that pushes `KResponsiveElementMixin`
into `ContentCard` in `<script>` part above `export default`
of `ContentCardGroupCarousel`.

Due to how files are bundled, the removed piece of code is evaluated
on the application start, no matter whether `ContentCardGroupCarousel`
is mounted or no. Consequently, `KResponsiveElementMixin` is unintentionally
used in all `ContentCards` in the whole app, not only in `ContentCardGroupCarousel`
itself. This means unnecessary performance losses whenever `ContentCard` is used
on a page because `KResponsiveElementMixin`s `ResizeSensor` causes render tree
layouts/reflows.

Regarding the intended usage for `ContentCard`s that are children
of `ContentCardGroupCarousel`, `elementWidth` and `elementHeight` provided
by `KResponsiveElementMixin` are not used in `ContentCard` at all,
and I haven't noticed any problems with the carousel during testing after
removing this code. Removed logic, including the warning in a comment,
is quite old. I assume that it might be obsolete.
@jonboiser jonboiser added this to the 0.15.0 milestone Jun 1, 2021
@jonboiser jonboiser merged commit 1aa47f1 into learningequality:develop Jun 1, 2021
@MisRob MisRob deleted the carousel-responsive-element-mixin branch June 12, 2021 12:47
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