PropertyRequiredMixin #4272
PropertyRequiredMixin #4272
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. |
| @@ -277,8 +276,6 @@ class InputColor extends FocusMixin(FormElementMixin(LocalizeCoreElement(LitElem | |||
|
|
|||
| super.updated(changedProperties); | |||
|
|
|||
| if (changedProperties.has('label') || changedProperties.has('type')) this._validateLabel(); | |||
There was a problem hiding this comment.
This use case forced me to add support for "dependent properties" -- essentially other properties to watch and re-validate when they change.
| @@ -128,4 +129,35 @@ describe('d2l-input-color', () => { | |||
|
|
|||
| }); | |||
|
|
|||
| describe('validation', () => { | |||
There was a problem hiding this comment.
Example of how a component can now test that the validation is set up properly.
| .to.throw(TypeError, createDefaultMessage(tag, 'attr1')); | ||
| }); | ||
|
|
||
| it('should work in a subclass/mixin', async() => { |
There was a problem hiding this comment.
This was a crazy case. When mixins or inheritence are present, the static properties need to be collected by traversing the inheritence tree. Loosly inspired by @eKoopmans' provider stuff.
|
|
||
| _initProperties(base) { | ||
| if (base === null) return; | ||
| this._initProperties(Object.getPrototypeOf(base)); |
There was a problem hiding this comment.
Neat, I figured there had to be a better/cleaner option than __proto__.
There was a problem hiding this comment.
Yeah was going to mention this option to you, as when I was looking up __proto__ the Mozilla docs had all kinds of scary deprecation warnings!
|
This is ready for review. As I mentioned in standup, I'm going to do a follow-up that converts more things to use this. |
helpers/error.js
Outdated
There was a problem hiding this comment.
After chatting with @dbatiste, we decided that it would be useful to centralize the formatting of errors originating from custom elements. That way we can ensure they're formatted consistently, always include the tag name in the same way and can optionally include the entire composed path.
This error now looks like:
TypeError: : "label" attribute is required. Path: "d2l-demo-page > main > div > slot > d2l-demo-snippet > div > div > div > slot > div > d2l-input-color". (@brightspace-ui/core:PropertyRequiredMixin)
There was a problem hiding this comment.
I'm guessing there isn't actually a double : :... or is there?
Anyway, I think this path will be really helpful to locate the source of some issues. 🎉
There was a problem hiding this comment.
Haha there is not a double :, not sure where that copy-paste mistake came from!
|
🎉 This PR is included in version 2.163.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is a mixin that can be leveraged whenever a component has a property/attribute that they want to treat as "required" -- something the developer consuming the component must set. This typically comes up on the accessibility side where we want to ensure labels are provided.
I've only implemented this for
Stringproperties, because I wasn't sure what it would mean for aBoolean(present or not?) or aNumberto be "required". If we have actual use cases, I'm happy to add support for that.