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

1603 lazy loading option for compound web component mode #3590

Merged
merged 39 commits into from
Jan 15, 2024

Conversation

camelCaseChris
Copy link
Contributor

@camelCaseChris camelCaseChris commented Dec 14, 2023

Description

Changes proposed in this pull request:

  • There is an option to activate lazy loading mode for a compound web component.
  • With lazy loading, after rendering the container for each compound item, the item is not immediately rendered. Instead, the container is added to an IntersectionObserver that triggers the rendering when the container is visible. To support this, each compound item container is rendered with a temporary height, so that not all containers are immediately visible in the viewport.
    • The temporary height is removed from the container after attaching the web component to the compound item.
    • The container might in some cases switch back to a height to 0, e.g. if the attached web component renders its contents asynchronously. But this is nothing we can solve but needs to be documented and is then up to the UI developers to (if even needed) adapt to through configuration or changes to the web components.

Discussion points:

  • Configuration? Current proposal:
    • compound.lazyLoadingOptions (object) options for lazy loading of the compound.
      • enabled (boolean) to activate lazy loading mode.
      • temporaryContainerHeight (CSS length string) height to apply as a default for all children of the compound. If not specified, a default fallback is used, currently "500px".
      • intersectionRootMargin (CSS length string), to be passed on to the IntersectionObserver
    • compound.children[].layoutConfig.temporaryContainerHeight (string) CSS height to apply to the individual children of the compound. Overrides the compound default.
  • Lazy loading for nested compounds?
    • Yes
    • To be checked: does IntersectionObserver work for things inside shadow DOM?
  • Lazy loading for custom renderers?
    • Setting the temporary height to the compound item containers may conflict with what custom renderers are doing. How to handle this?
    • For now: "solve by documentation + configuration".
      • Documentation: describe what lazy load is doing.
      • Configuration: offer an option compound.lazyLoadingOptions.noTemporaryContainerHeight to suppress the setting/removing of the temporary height to the compound item containers.
  • Central place to store constants, like the default fallback for compound.temporaryContainerHeight?
    • In class
  • Standards for error handling?
    • Foreseen, developer errors: log to console
    • Relevant for end-users: internal alert mechanism
  • Clean up console logging and comments

Known issues:

  • Even with big temporary heights for the compound item containers, it can happen that surprisingly many items are rendered right away. This needs further investigation.

Related issue(s)
Fixes #1603.

@camelCaseChris camelCaseChris marked this pull request as ready for review January 9, 2024 15:15
@JohannesDoberer JohannesDoberer self-assigned this Jan 10, 2024
Copy link
Contributor

@JohannesDoberer JohannesDoberer left a comment

Choose a reason for hiding this comment

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

It looks good to me!
Good Job 👍 .
Small comments regarding documentation.

* @param {*} context the luigi node context
*/
renderWebComponentCompound(navNode, wc_container, extendedContext) {
const useLazyLoading = navNode.compound?.lazyLoadingOptions?.enabled === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please document this settings object lazyLoadingOptions.
E.g here and maybe it is worth to mention this object in this document as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done...

});
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a description to the function what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done...

compoundItemContainer.style.height = temporaryContainerHeight;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a description to the function what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done...

Copy link
Contributor

@JohannesDoberer JohannesDoberer left a comment

Choose a reason for hiding this comment

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

LGTM 👍 !

@camelCaseChris camelCaseChris merged commit 428fa64 into main Jan 15, 2024
@camelCaseChris camelCaseChris deleted the 1603-wc-lazy-load branch January 15, 2024 14:08
@JohannesDoberer JohannesDoberer added the enhancement New feature or request label Jan 23, 2024
This was referenced Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LazyLoading for web components MFEs
2 participants