microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.17k stars 587 forks source link

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

Open m-akinc opened 2 months ago

m-akinc commented 2 months ago

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:

🎫 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

m-akinc commented 1 month ago

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

m-akinc commented 1 month ago

@janechu is there anything I can do to help get this in?

janechu commented 1 month ago

@janechu is there anything I can do to help get this in?

LGTM, sorry for the delay!

m-akinc commented 1 month ago

@chrisdholt while we've got some momentum, would be nice to get a review on this. 😊

m-akinc commented 1 month ago

@janechu are you comfortable merging this in? @chrisdholt do you want to review?