Fix translations #130
Fix translations #130
Conversation
| @@ -92,8 +92,7 @@ | |||
| "min": 0, | |||
| "max": 100, | |||
| "default": 100, | |||
| "unit": "%", | |||
| "label": "" | |||
There was a problem hiding this comment.
label is required for number fields in the specification. I'd prefer it to be kept as is, instead of potentially having to null check a value that per definition should exist.
There was a problem hiding this comment.
H5P core does not enforce the label to be set. The H5P CLI tool complains when validating the translation files if it finds empty text fields. Also, you will find the very same structure without the empty label for the "Overall Feedback" group in all the other content types. The null check is done in the Table-List widget that this is used in, no need to do that oneself.
Feel free to discuss with the H5P core team why the property is claimed to be mandatory while they're not enforcing it and against their own specification, their tool complains.
There was a problem hiding this comment.
Frustrating to say the least 😢 h5p-types expects the label to be set and has problems inferring params' type if a field doesn't follow the spec. I'll add functionality for inferring partial fields at some point.
There was a problem hiding this comment.
I didn't say I like it that way, but I don't think NDLA requested the use of TypeScript.
/cc @janlindso to rule on this one.
There was a problem hiding this comment.
No, I'm not saying I find the code in this PR frustrating, it's H5P Core I'm frustrated about.
There was a problem hiding this comment.
I got that :-D I don't like a couple of things and the way they are handled in H5P core, but I create pull requests to suggest solutions or start discussions (e.g. https://h5p.org/node/1242568 or h5p/h5p-editor-php-library#134) - and until that's resolved, I stick to what I am supposed to stick to or need to put effort into workarounds.
There was a problem hiding this comment.
It's so nice that you put the effort into starting these discussions and creating the PRs that would solve issues! I have stopped considering those as options as they seldom are followed up:( I'd really like to see h5p-editor-php-library#134 being merged, though!!!
Anyways, I'll create a fix that makes h5p-types a little more understanding. If NDLA wants us to rely less on TS, we'll find a compromise to JS-ify at least a little ☀️
There was a problem hiding this comment.
I am used running my head into walls. Again and again. 😅
There was a problem hiding this comment.
I have reached out to H5P Group for clarification now, but I'd assume that the specification simply is outdated. The editor form of H5P itself does not assume that the label is set and uses a guard: https://github.com/otacke/h5p-editor-php-library/blob/53dc7bdc57b17f5a0d55a8871a36d1b1bd631e70/scripts/h5peditor-library.js#L105-L114
|
🎉 |
|
I think this is good for now. And I also think it's good to rely less on TS as it would probably make the code even more available to less advanced developers. |
No description provided.