Migrate interactive app to JSONForms #30
Conversation
* Remove redux from react-seed * Replace initialData with jsonformsData * Add yalc to gitignore * Add clear data button * Replace data attribute with uischema attribute * Refactor Rating to functional component * Remove not needed styles * Clear data instead of reset data * Update README * Showcases labels and titles * Adjust theme to give controls more space * Remove horizontal scrollbar * Update to JSON Forms 2.5.0 Co-authored-by: Stefan Dirix <[email protected]>
|
There was a problem hiding this comment.
Changing from 'approve' to 'request changes' after seeing Chris' comment
|
I had to ), but it does work for me locally |
|
Could you change standards-checklist/.github/workflows/ghpages.yml Lines 24 to 32 in f3418d9 |
|
Do we have a deployment somewhere to test? I tried pushing to my fork and this is what I get: https://effigies.github.io/standards-checklist/ |
nx10#1 should fix deployment |
There was a problem hiding this comment.
-
I didn't notice in my initial review, but these changes lose the
clarifications from the UI. E.g,before after 
-
I think re: "List of Links/URLs can now be added directly" / ✨ Add metadata to form #23 (this PR should close ✨ Add metadata to form #23 if merged) that we want to include a timestamp from when the review was completed and some record of who completed the review. These can be separate PRs, especially since I don't think we ever decided how we want to authenticate / credit reviewers.
-
This PR doesn't include UI for comments, but this PR should close ✨ Add comment boxes to checklist items #22 if merged (UI for comments can also be a separate PR)
-
I think we want the bronze sections to start out expanded for a new checklist and for sections to automatically collapse when completed and subsequent sections to expand (but ♻️ Autocollapse completed sections #21 was opened over a year ago, so maybe we don't care? Either way, this PR should also close ♻️ Autocollapse completed sections #21). The initial clicks to open the checklists seem like unnecessary user friction to me.
-
Do we want to address this in this PR or later?
Do we need one more level of depth for the items, to include the boolean and 💄 Add UI for comments #18?
Exactly, it would look something like this:
"2": { "type": "object", "title": "Documentation is up to date with version of software", "properties": { "value": { "type": "boolean", "default": false } "comment": { "type": "string", "default": "" } } },
To avoid repetition this could be $def-ined once and then used for every item.
-
Do we want to address this in this PR or later?
For some items, like this one, where the answer could be "not applicable", I think we'll need
"anyOf": [ { "type": "boolean" }, { "type": "string", "enum": ["not applicable"] } ]
Makes sense!
Not applicable could also be aboolean -
These questions / comments are still relevant
-
Is the idea to move away from the JSON-LD encoding in checklists/checklist.json or to have it also encoded in JSON schema?
-
The current directory structure is with the idea that the checklist content will be versioned separately from the checklist interface. I think that's the right design, but I don't know how to do that without housing the schema in a sibling directory tree with its own
package.json.
-
👷 Deploy jsonforms in relative path and use jsonforms-react-seed README
|
Regarding 1: I have added the
Regarding the other points I am waiting for judgement / decisions. |
|
@shnizzedy thanks for the thoughtful comments!
Would love if we could get this "live" soon, and then there are certainly kinks that will need to be ironed out as we use it more! |
Of course this is easy to add, but I assume opening separate PRs for this and Jon's other points would make them easier for you all to review. |
|
I've poked around on a rendered site, and things overall look good, but haven't had the spare cycles to really do critique. If Jon and Greg are happy, I say let's merge. |
|
Yeah, I'm happy with merging and opening issues for the open threads here. To answer a few questions / clarify a couple points:
3 is adding comment support to the UI, 5 is adding comment support to the schema
6 is adding "n/a" as a specific selectable option where relevant
The current schema is already JSON-LD flowchart LR
LD["JSON-LD"] --> UI
; this PR introduces a JSON Schema document as a separate, parallel source of truth. flowchart LR
LD["JSON-LD"]
Schema["JSON Schema"] --> UI
I'm suggesting we keep the JSON-LD as the source of truth and autogenerate the JSON Schema for the UI from the JSON-LD flowchart LR
LD["JSON-LD"] --automation--> Schema["JSON Schema"] --> UI
All that said, I agree these are all issues to open / cans to kick once this PR is merged in. |
There was a problem hiding this comment.
Haven't had the time to do a deep review, but overall this looks great, and fulfills the goal of actually being able to use the checklist while reviewing a project - I'm also in favor of kicking the can on the rest of the missing pieces. thanks @nx10 !!
This PR migrates the interactive app to JSONForms.