-
Notifications
You must be signed in to change notification settings - Fork 585
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
base: archives/fast-element-1
Are you sure you want to change the base?
fix(fast-element1): fix design token updates when attaching/detaching nodes and deleting values #6960
Conversation
@@ -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 () => { |
There was a problem hiding this comment.
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", () => { |
There was a problem hiding this comment.
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.
@chrisdholt No rush, but it's been a couple weeks since I posted this, so I thought I'd give you a ping. |
Pull Request
π Description
This PR addresses a couple design-token-related bugs:
T
(with CSS variable--T
)T
to "10" for both<my-child>
and<my-parent>
<my-child>
's value is the same as the inherited value, the value is not reflected to CSS on<my-child>
T
to "20" for<my-other-parent>
<my-child>
under<my-other-parent>
(detach, then append)<my-child>
, the value of the CSS variable--T
should be "10" for it, but it is not. It is "20", which is incorrect.<my-child>
differs from<my-other-parent>
, it should reflect the value to CSS. It doesn't. β<my-element></my-element>
T
(with CSS variable--T
)T
to "10" for<my-element>
T
from<my-element>
<my-element>
should not have any value for--T
, but it is still defined as "10".Also:
.only
from a combobox test that was apparently submitted on accidentconsole.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
$ yarn change