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

refactor(dropdown): extend updateComplete timing for overlay oven/close #708

Merged
merged 1 commit into from
Jun 1, 2020

Conversation

Westbrook
Copy link
Contributor

Description

  • allows sp-dropdown to lazy load the Overlay and sp-popover code
  • wraps that laziness into the updateComplete lifecycle promise, so that timing-based interactions with the element can be managed with the appropriate fidelity

Motivation and Context

  • users should be able to prevent unused code from being downloaded by their applications
  • when doing so, those same users should be able to time interactions based on the availability of that code

How Has This Been Tested?

  • updated unit tests
  • manually build/test in westbrook/11ty

Types of changes

  • refactor (update interface to render timing)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook force-pushed the westbrook/dropdown branch 4 times, most recently from 1ae8a1d to 5acb5ca Compare May 30, 2020 14:19
@@ -256,16 +257,39 @@ export class DropdownBase extends Focusable {
if (menuWidth) {
this.popover.style.setProperty('width', menuWidth);
}
Copy link
Collaborator

@andrewhatran andrewhatran Jun 1, 2020

Choose a reason for hiding this comment

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

I understand that we're lazy loading Overlay so that we can prevent unused code from being downloaded.

However, since every dropdown contains an overlay, wouldn't Overlay be imported every time a dropdown is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%, Overlay is a hard requirement of using the functionality that is delivered by an sp-dropdown element. However, lazy loading isn't only about preventing unused code; that's more directly a responsibility of tree shaking and side effect management (see #679 that looks to enhance out approach to that).

In this case, Overlay isn't a requirement of displaying the dropdown element when closed, and if you don't need to interact with the dropdown, this would allow the page never to download that JS. As an example, checkout the documentation site: https://opensource.adobe.com/spectrum-web-components/ unless you specifically interact with the theme customization UI, or are looking to review the dropdown documentation, there is no need to load the Overlay scripts.

In the case that you do need the Overlay scripts, it is highly unlikely that you'd interact with the dropdown immediately (e.g. load the page with the dropdown open by default, an,. if needed, that can be managed into a single bundle externally). This change looks to ensure that we give download/parse/run priority to JS that is required as the page is rendered by moving this Overlay into the dynamic dependency graph as opposed to the static dependency graph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, that checks out. Giving priority to JS when the dependency is required rather than at construction makes a lot of sense.

It makes even more sense for disabled dropdowns.

@andrewhatran andrewhatran self-requested a review June 1, 2020 21:09
Copy link
Collaborator

@andrewhatran andrewhatran left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Westbrook Westbrook merged commit a616310 into master Jun 1, 2020
@Westbrook Westbrook deleted the westbrook/dropdown branch June 1, 2020 21:10
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