GAUD-10267: use reactive state properties#7225
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| text: { type: String }, | ||
| _activeDimensionKey: { type: String, attribute: false }, | ||
| _dimensions: { type: Array, attribute: false }, | ||
| _activeDimensionKey: { state: true }, |
There was a problem hiding this comment.
Hmm, not amazing but I think setting the property will still work:
https://github.com/Brightspace/d2l-manage-courses-ui/blob/6686d7278ab57c904d63e9fd4cf873290a404c0e/test/d2l-source-courses-view.test.js#L106
| _activeDimensionKey: { type: String, attribute: false }, | ||
| _dimensions: { type: Array, attribute: false }, | ||
| _activeDimensionKey: { state: true }, | ||
| _dimensions: { state: true }, |
There was a problem hiding this comment.
Also not great:
- https://github.com/Brightspace/discovery-ui/blob/147a10e0eb68bd77475b5e2f7f6122d1d74e863d/test/discovery-my-requests.test.js#L455
- https://github.com/Brightspace/workforce-ui/blob/7f106c04b1237d090145e5aef5eccbb540b00d3e/test/vdiff/scheduled-learning-progress.vdiff.js#L147
And this one of ours might actually break:
So may need to undo this one.
There was a problem hiding this comment.
I don't think changing to state: true will cause breakage here, but maybe I am missing something.
But yeah, seeing consumers calling private members makes me want to double-down on switching all the non-reactive "private" members to use #.
There was a problem hiding this comment.
Agreed 💯. I've confirmed that these still work -- there's actually nothing in Lit that prevents them from being set as properties, which makes sense.
I feel like it's still worthwhile to make the change, even though consumers are referencing these.
|
🎉 This PR is included in version 3.271.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Converts core from the old way to do "internal" state to use reactive state properties.
The only potential issue with this conversion would be if someone was doing a property binding (e.g.
<d2l-floating-buttons ._containerMarginLeft="5px">), which... hopefully isn't being done because that's gross.