-
Notifications
You must be signed in to change notification settings - Fork 589
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: Add decorator for configuring CSS CustomStateSet
s
#6967
base: master
Are you sure you want to change the base?
Conversation
One option here would be to expose state on the Another thing I notice is that this decorator mixes observability with css state. One thing to consider here is that decorators can be used together, so authors should be able to do: @observable
@cssState
property: string = ""
// or
@attr
@cssState
attr: string = "" This approach would keep the behaviors more isolated and allow usage of CSS state w/o the overhead of property observability if desired. You might need to upgrade TS to do that though... I'm not sure if this version exposes the underlying property descriptor or not. You would probably need to update observable to not just clobber existing property descriptors as well. |
Looking at the docs, it looks like property decorators don't get the descriptor passed to them so chaining may be difficult but I don't think it's impossible. In my initial testing for composing the different decorators, which ever decorator is applied to the property second clobbers the first. I think both this decorator and observable would need to first check if an accessor already exists and reflect calls to it rather than just overriding it completely, but I'm not entirely sure if it can be done with the Reflect API. |
Some new things that have come up around this. I think @nicholasrice's suggestions on making custom states part of the definition and What I'm not sure about is making both this new decorator and observable composable. I managed to get a working prototype of composable decorators that don't just clobber the The benefit to making The second tradeoff above could be alleviated by exporting some utilities for creating composable decorators which would also enable FAST to have a lot of control over user decorators created with said utilities, but I'm not sure what that would look like yet. @janechu I'd love to hear your thoughts on all of this. I'm not sure what the timeline is for getting v2 out the door, so perhaps we keep this change in a draft state and iterate on it for v3. |
I believe @radium-v is going to have more feedback on the actual implementation, for 2.0.0 launch we're still in progress on updating the documentation site, hopefully that will be done very soon. If this is going to go in, either as part of 2.0.0 or as a feature release I'm not particular, however it should have a section on the site. I'll link to the updated documentation PR once it's up. |
Hi 👋🏼 I'm still learning about anecdote starts here In this particular case, support for :host(:is(:--focus-visible, :state(--focus-visible)):focus-within) { ...Which is admittedly pretty insane, but it does work. It also lets me avoid using an arbitrary attribute. There are of course compatibility concerns, which is why the state is anecdote stops here For Fluent components, I've been sticking to Additionally, if
|
For this reason, I think this change should be part of v3. v2 is too close to release for us to be introducing breaking changes especially considering v2 beta has been stable for quite some time now. Now, we could add elementInternals to FASTElement rather than the controller which should soften the impact this change has a bit, but I'm not sure what the implications of that would be yet. On the compatibility aspect I think we should be able to detect which version of the spec is supported within the decorator and set the state as a string or dashed-ident accordingly. Writing the actual styles would be up to the component author since the decorator is only concerned with adding/removing the state. |
Pull Request
📖 Description
This PR adds a new decorator for configuring
boolean
properties as CSS custom states. The decorator will attempt to callattachInternals
on the element if it hasn't already and then create the configured property as an observable property with a custom setter that handles setting and removing the custom state from the element'sElementInternals.states
set.update:
The bulk of the decorator's logic has been moved to a separate
CSSStateDefintion
class in order to more easily handle notifying subscribers of changes and to support propertyChanged callbacks for the decorated properties. The implementation of the class follows a similar pattern used by@attr
.Usage
🎫 Issues
👩💻 Reviewer Notes
Some open questions:
I'm not sure if attaching internals within
CSSStateDefinition.setValue
is the best place for it. The issue is that in order to know ifElementInternals
has been attached to the element, it needs to be assigned to a property and the logic to detect it cannot live in the decorator due to the decorator not having access to the component instance. We could implement a similar solution as@attr
where it implements ametadataLocator
to set the configuration for the component's type and then later instantiates the custom states during element construction. This is a larger change that would affectFASTElement
.In order to more consistently detect if
ElementInternals
has already been attached to the source element, I think eitherFASTElement
orElementController
would need to be updated with an optionalelementInternals
property. An override forattachInternals
would likely be needed to set this new property so that component authors can just callthis.attachInternals()
and not need to worry about manually setting a class member for it.I'm not sure the components folder is the best location for this, perhaps it is more related to styling?
I haven't seen
CustomStateSet
s used much in the wild yet and any usage I have seen has been limited to boolean properties since custom states are similar to HTML boolean attributes where their true/false states correlate with the existence of the attribute or in this case, the value passed tostate()
selector causing styles to be applied. If support for other primitive types should exist, then we should discuss what that looks like.It would seem TypeScript (or the version FAST uses) does not yet support
CustomStateSet
onElementInternals
so this PR also includes a small type polyfill forElementInternals.states
.fast-element
does not have aglobal.d.ts
file which is typically where global interfaces reside, so for now I defined the modified interface within the same file as the decorator.📑 Test Plan
I'm having difficulty adding tests around this change due to some issues with the test environment. Perhaps @radium-v could help take a look?
✅ Checklist
General
$ yarn change
⏭ Next Steps
A nice enhancement would be to add an optional configuration to the decorator similar to
@attr
's configuration where the name of the applied state in CSS can be defined differently than the property on the class.