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

feat(fast-element): move template instance timing and enable dynamic templates and styles #3621

Merged

Conversation

EisenbergEffect
Copy link
Contributor

Description

This PR moves the timing of the FASTElement template instantiation out of the constructor and into the connected callback. Additionally, new hooks for dynamic template and style selection have been added as well as the capability to hot swap templates and styles at any point for any FASTElement.

Motivation & context

This PR covers the work in the fast-element phase of #3361 by moving the template creation timing to the connected callback and introducing the resolveTemplate() and resolveStyles() hooks. Additionally, template and styles properties have been added to Controller so that the template and base styles for any FASTElement can be changed at any time. This enables the implementation of FoundationElement as called out in #3361 as well as scenarios around a future router web component, and more dynamic or conditional element rendering. Furthermore, the timing change helps ensure that elements are properly defined time-wise in more complex scenarios, addressing some bugs our partners have been seeing.

While working on this change, some additional refactoring was done around element definitions, in the attempt to consolidate logic and create a better API. A few new APIs were made public or improved on templates and views, as needed for the functionality of dynamic templates and styles.

In addition to these changes, Controller tests have been added to validate its correct function during construction, connection, and operation. Documentation has been updated and extended in several places to help bring further clarity around the lifecycle and new capabilities.

Reviewer Request: I would very much appreciate someone reviewing the fast-components via Storybook to ensure that no unexpected behavior has been introduced as a result of this change.

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

While this doesn't appear to cause current functionality to break, it's possible that there are some unknown side-effects of the timing change. Additionally, some low-level APIs in fast-element have changes during the refactor. This is not likely to affect anyone though.

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Comment on lines +169 to +173
It is during the `connectedCallback` phase of the Custom Element lifecycle that `FASTElement` adds the element's styles. The styles are only added the first time the element is connected.

In most cases, the styles that `FASTElement` renders are determined by the `styles` property of the Custom Element's configuration. However, you can also implement a method on your Custom Element class named `resolveStyles()` that returns an `ElementStyles` instance. If this method is present, it will be called during `connectedCallback` to obtain the styles to use. This allows the element author to dynamically select completely different styles based on the state of the element at the time of connection.

In addition to dynamic style selection during the `connectedCallback`, the `$fastController` property of `FASTElement` enables dynamically changing the styles at any time through setting the controller's `styles` property to any valid styles.
Copy link
Member

Choose a reason for hiding this comment

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

This is a great addition

:::

In most cases, the template that `FASTElement` renders is determined by the `template` property of the Custom Element's configuration. However, you can also implement a method on your Custom Element class named `resolveTemplate()` that returns a template instance. If this method is present, it will be called during `connectedCallback` to obtain the template to use. This allows the element author to dynamically select completely different templates based on the state of the element at the time of connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to conditionally override the template property from this method? For instance, does the controller defer to the template property template if the returned value from this is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you return null from resolveTemplate() then it won't render any template. But you can come back later and set $fastController.template and that will trigger a render. Due to the timing of the template location now, you could have your resolveTemplate() implementation check the state of its attrs and observables and use that to choose one or another template, because that should all be setup by the time the hook gets called.

Does that answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! I am pondering if that capability could be useful but can't think of a solid use-case at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fearful of the internal logic getting a bit complicated and hard to reason about. So, I kept it to a straight priority. First check if the property has been set, then check the custom hook, then check the static definition. I think any advanced scenario can be handled by having code set the property as needed. This also allows the property to be surfaced on FoundationElement as an observable and be bound on a per instance basis as well, which would enable per element template or style overrides with very little effort.

}

if (template) {
// we have a new template to render to the Light
Copy link
Contributor

Choose a reason for hiding this comment

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

heh - the Light!

Copy link
Contributor Author

@EisenbergEffect EisenbergEffect Aug 4, 2020

Choose a reason for hiding this comment

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

Oops. 🤣 Old comment left around after I refactored. I pushed a commit that adds some better internal comments on what's going on throughout.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I've seen, written, and found worse comments @EisenbergEffect 😆

Copy link
Contributor

@nicholasrice nicholasrice left a comment

Choose a reason for hiding this comment

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

Looks great! Just had one question above

@EisenbergEffect EisenbergEffect merged commit 2e73088 into master Aug 5, 2020
@EisenbergEffect EisenbergEffect deleted the users/eisenbergeffect/rework-template-create-timing branch August 5, 2020 16:43
@EisenbergEffect EisenbergEffect added this to the Release 06 milestone Aug 11, 2020
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.

5 participants