added some fields and todos from SWHAPPE example https://raw.githubusercontent.com/Unipisa/SWHAP-TEMPLATE/master/metadata/codemeta.json
Details
Details
Diff Detail
Diff Detail
- Repository
- rDWCM CodeMeta Generator (mirror)
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
| codemeta_generation.js | ||
|---|---|---|
| 44 | you can add operatingSystem if you fix line 86 :-) | |
| index.html | ||
| 37 | I looked at the schema.org definition of identifier (https://schema.org/identifier), <div property="identifier" typeof="PropertyValue">
<span property="propertyID">DOI</span>:
<span property="value">10.151.xxxxx</span>
</div>we can use that with identifier-type and identifier-value to have a clearer idea of what needs to be in the input. | |
| 85 | Seems there are two properties here,operatingSystem and runtimePlatform The description of runtimePlatform is more about dependencies:
| |
| 101 | the text should be isPartOf instead of funding | |
Comment Actions
This is already done, else you wouldn't be able to read the code :)
| dynamic_form.js | ||
|---|---|---|
| 38–41 | nitpick: indentation of the <p> block | |
| index.html | ||
| 74 | The id of this fieldset should be changed to fieldsetAdditionalInfo. (And possibly, move the fieldset at the end, as it's for fields that don't fit other ones) | |
| 79 | Description should be in the text, not the placeholder. Placeholders are for examples. | |
| 85 | Same, placeholder should be an example | |
| 91 | You could use a datalist for this, so it auto-completes when users write it. Example: https://developer.mozilla.org/fr/docs/Web/HTML/Element/datalist Possibly with a validator pattern (as for dateCreated) if we want to validate it. | |
| 94–98 | We should add support for multiple languages, eg. by splitting on commas when exporting. | |
| 103 | Wrong format, schema.org requires this to be a CreativeWork. So the best way to handle this is to ask for an URI, and output: "isPartOf": {
"@id": "https://the provided uri",
} | |
This comment was removed by scatenag.
This comment was removed by scatenag.
Comment Actions
description is not an HTML tag, instead you should use a CSS class (eg. <div class="field-description">, and use .field-description { ... } in the .css file)
Also, as these descriptions are added after the input field (which makes sense visually), it means that screen-readers will read the description after the user filled the field (or skipped it), which is not optimal. To fix this, you could add a unique id= to each of the descriptions, and use the aria-describedby attribute on input elements; so screen-readers can locate the description for the field. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-describedby_attribute
| dynamic_form.js | ||
|---|---|---|
| 38–41 | Still not done; the label and input are inside the p, so it should be: <p>
<label for="${personPrefix}_affiliation">Affiliation</label>
<input type="url" id="${personPrefix}_affiliation" name="${personPrefix}_affiliation"
placeholder="Department of Computer Science, University of Pisa" />
</p> | |
| index.html | ||
| 191–240 | Could you move this before the author/contributors fieldsets, to optimize space, as they are inline-blocks? | |
| style.css | ||
| 13 | Could you add #fieldsetAdditionalInfo to this list? (same reason as above) | |
| 36–38 | Don't override the default color for valid input; because if the user uses a dark theme (ie. with text in white-on-black), then this text becomes unreadable (black-on-black) | |
| 36–41 | Good addition. Could you remove the indentation before the }? | |
Comment Actions
- revision and aria-describedby on description
I hope I did the aria-describedby correctly
(I see no effect ... :-/ ).
Apart from finishing the fieldset with missing array-values...
we are thinking about getting this tool running somewhere and put a link in the SWHAPPE and in the SWHAP guide...
Have you thought about hosting this tool?
Could you host this somewhere?
Comment Actions
Sorry, I missed your comment when reviewing the changes.
You did :)
(I see no effect ... :-/ ).
It's not supposed to be shown visually, it's an attribute used by some screen readers (look for the "aria-describedby" table here: https://www.powermapper.com/tests/screen-readers/aria/ for a list of supporting screen readers)
Apart from finishing the fieldset with missing array-values...
We can do that in an other diff.
we are thinking about getting this tool running somewhere and put a link in the SWHAPPE and in the SWHAP guide...
Have you thought about hosting this tool?
Could you host this somewhere?
ping @moranegg
Comment Actions
@scatenag Could you land this diff? (see https://wiki.softwareheritage.org/wiki/Code_review_in_Phabricator#Landing_your_change_onto_master )
Then rebase/arc diff --update D2351. That will allow me to review it and see only its new changes.
Comment Actions
Yes ...but, initially I did understand "ready to land" mean ..
Now..i got this
arc land --revision D2216
BUILDS PASSED Harbormaster builds for the active diff completed successfully.
PUSHING Pushing changes to "origin/master".
git@forge.softwareheritage.org: Permission denied (publickey,keyboard-interactive).
fatal: Could not read from remote repository.
..
Comment Actions
Did you configure a public SSH key in your account? https://wiki.softwareheritage.org/wiki/Code_review_in_Phabricator#SSH_key_for_pushes