c-suh
commented
5 years ago Addressed and released in https://github.com/NASA-PDS-Incubator/feedback-widget/releases/tag/v1.1.1
Closed acraugh closed 5 years ago
c-suh
commented
5 years ago Addressed and released in https://github.com/NASA-PDS-Incubator/feedback-widget/releases/tag/v1.1.1
The README looks pretty good, but there are a couple of issues in the "Configured Variables" table:
Footnote (5) tells users to enter integer values only, not units like "px" or "vw". But every single corresponding "default" value has explicit units shown, and there is no indication of what units will be applied to the naked integer.
What you have now is likely to lead to needless confusion and frustration. If the user cannot select units, then the default values in this table shouldn't show them, and the description should name the (de facto required) unit explicitly. But if the user cannot select units, then there shouldn't be any units in the config.js file, either - you're basically instructing users to remove the units that are already in there! If what you mean is "Do not change the units that are in config.js", then say that - but expect users to change them anyway.
The "placement -offset" line shows a value range of "[0,95)" [sic]. The use of an opening bracket with a closing parenthesis looks like a typo; if it isn't one it needs to be explained.
The set of values itself is also somewhat mysterious. I don't see why "0" would be an allowed value on one extreme, but "100" is not allowed on the other. It's also not immediately clear to me why the unit is "vh" regardless of whether the widget is placed top/bottom or left/right. Perhaps it is clear to CSS programmers?
This table does not follow the order or the syntax in the config.js file, and it's not sorted in alphabetical order, either. If the table were any longer it would be kind of annoying to have to scroll back and forth looking for things. Still, one order or the other should be applied.
The structure of the "followUpLinks" section is more complex than what is in the table.That's not a major issue for people who know JSON, but that does not necessarily describe people who would be tasked with implementing this widget.
On the whole, I'd re-order the table to match config.js, and I would expand the content of the "followUpLinks" line the same way as has been done for "size" and "placement". Then I'd expand the description to include a) what in the JSON structure can be changed ("url" only? "title"?); and b) to remove the implication that anything in there should be a multivalued list of URLs - unless that is actually valid.