GAUD-10267: add transform to fix wca#7219
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. |
| let mode = 'code'; | ||
| let quote; | ||
|
|
||
| for (let index = start; index < content.length; index++) { |
| @@ -2,74 +2,72 @@ import { FocusMixin } from '../../mixins/focus/focus-mixin.js'; | |||
|
|
|||
| export const ButtonMixin = superclass => class extends FocusMixin(superclass) { | |||
|
|
|||
There was a problem hiding this comment.
I had previously left these static getters in because our validate-wca.js script was complaining that they were missing descriptions... which should have set off alarms about the issue.
The good news is that we can now switch these to use the non-getter version.
|
|
||
| await runWca(join(temporaryDirectory, '**/*.js')); | ||
|
|
||
| await restoreCustomElementsPaths(temporaryDirectory); |
There was a problem hiding this comment.
I've done a side-by-side diff of custom-elements.json from before the static props stuff merged against this. Everything's identical with the exception of a few things now correctly being documented where we were already using static props (like <d2l-page-*> stuff).
There was a problem hiding this comment.
Committing custom-elements.json doesn't make sense and will just be too noisy right? The smoke tests in the docs site did their job, and we've been fine up to this point, but also feels easy to miss a problem.
There was a problem hiding this comment.
It would be very noisy, and I think we'd need to build an action similar to the translation formatter / vdiff that would PR into your PR with the changes? Otherwise we're relying on devs running build and committing it.
There was a problem hiding this comment.
That could all live in the docs site instead, rather than committing it here. But yeah, it would probably require a dev review almost every day 😬. Can't decide if that's worth it
There was a problem hiding this comment.
So I don't think we can fully move it to the docs site because component-usage-metrics also uses it.
|
Idk, I feel like the change to the getters just wasn't worth adding this extra complexity. |
It's not great, but also this uncovered the fact that we'd been using the new getter format in various places and it was broken and we just didn't know yet. Also The issue is more with |
| if (mode === 'block-comment') { | ||
| if (character === '*' && nextCharacter === '/') { | ||
| mode = 'code'; | ||
| index++; |
There was a problem hiding this comment.
why here you increment the index and not only do the continue;?
Oh nvm, it is because the next character is the known / which we want to skip I guess
4 participants
When the properties were switched from static getters, the web component analyzer stopped recognizing them as "attributes", which ultimately lead to the Daylight Site losing all the documentation for the properties.
This introduces a script that:
static properties = { ... };back tostatic get properties() { return { ... }; }custom-elements.jsonsince they'll now have the temp directory in themCurrently this issue only impacts core (Labs and
lms-coredon't usewca), but if we wanted to reuse this elsewhere we could bundle it as a binary with core.