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

fix(fast-element1): fix design token updates when attaching/detaching nodes and deleting values #6960

Open
wants to merge 4 commits into
base: archives/fast-element-1
Choose a base branch
from

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented May 14, 2024

Pull Request

πŸ“– Description

This PR addresses a couple design-token-related bugs:

  1. Removing/adding a child element does not update the CSS token reflection of that child. E.g.
    • Starting with:
    <my-parent>
       <my-child></my-child>
    </my-parent>
    <my-other-parent></my-other-parent>
    • Define a token T (with CSS variable --T)
    • Explicitly set T to "10" for both <my-child> and <my-parent>
      • because <my-child>'s value is the same as the inherited value, the value is not reflected to CSS on <my-child>
    • Explicitly set T to "20" for <my-other-parent>
    • Reparent <my-child> under <my-other-parent> (detach, then append)
    • Because the value "10" is explicitly set on <my-child>, the value of the CSS variable --T should be "10" for it, but it is not. It is "20", which is incorrect.
      • since the value for <my-child> differs from <my-other-parent>, it should reflect the value to CSS. It doesn't. ❌
  2. Deleting a token value from an element should notify affected elements (and update CSS reflection). E.g.
    • Starting with: <my-element></my-element>
    • Define a token T (with CSS variable --T)
    • Explicitly set T to "10" for <my-element>
    • Delete T from <my-element>
    • <my-element> should not have any value for --T, but it is still defined as "10".

Also:

  • Removed .only from a combobox test that was apparently submitted on accident
  • Now catching errors thrown from derived token evaluation (and logging via console.error). Now that we are properly triggering token re-evaluations when detaching elements from the DOM, a derived token may throw an error during evaluation, because it depended on an inherited token value. It seems better to print an error than to let this derail execution.

🎫 Issues

N/A

πŸ‘©β€πŸ’» Reviewer Notes

Stackblitz demo of bug numbered 1 in description.
Stackblitz demo of bug numbered 2 in description.

πŸ“‘ Test Plan

Added additional test cases.

βœ… Checklist

General

  • I have included a change request file using $ yarn change
  • 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.

@@ -640,7 +715,7 @@ describe("A DesignToken", () => {
await DOM.nextUpdate();
expect(handleChange).to.have.been.called.once;
});
it("should notify a subscriber for a token after being appended to a parent with a different token value than the previous context", async () => {
it("should notify a subscriber for a derived token after being appended to a parent with a different token value than the previous context", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just disambiguating this test from another right next to it that had the same name/descriptor.

@@ -877,6 +963,37 @@ describe("A DesignToken", () => {
removeElement(ancestor);
});

it("should notify an un-targeted subscriber for each element whose value changes", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other existing test doesn't verify this, so I added the test case.

@m-akinc
Copy link
Contributor Author

m-akinc commented May 28, 2024

@chrisdholt No rush, but it's been a couple weeks since I posted this, so I thought I'd give you a ping.

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.

None yet

1 participant