Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON Editor and Schema validation #1867

Merged
merged 253 commits into from
Nov 12, 2020
Merged

JSON Editor and Schema validation #1867

merged 253 commits into from
Nov 12, 2020

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Oct 26, 2020

Description

JSON editor implemented as a separate component. Root components are components/JsonEditor/JsonEditor.js and components/JsonEditor/State.js

Editor is a list of two-column tables. Every table is a nested level of JSON.

The hierarchy of components is the following:

  1. JsonEditor and State are root components
  2. Column, it's a two-column table actually. Represents one nested level of JSON
  3. Row, AddRow/AddArrayItem. Represents key/value pair or a control to add new rows
  4. Cell. Represents key or value wrapper
  5. Input and Preview. Edit and show key or value
  6. Note is like MUI InputAdornment for both Input and Preview

For a table implementation, I used react-table. It's almost useless in this context, to be honest, and I think we should get rid of it later.

UI/UX feedback

  • 1. show a placeholder / skeleton for the json editor while the schema is loading
  • 2. menus on key cells dont open on required fields, should we hide them?
  • 3. num, str, etc. helper strings dont seem intuitive enough, i'd rather go for more user-friendly captions e.g. Number, String, etc
  • 4. the schema gets refetched and json editor flashes when switching btw key-value and text modes
  • 5. keys can't be edited after being entered -- that's a deliberate decision, but seems a little confusing, so maybe we can revisit it moving forward
  • 6. "click to edit" native browser tooltip overlaps our "value should be ..." tooltip
  • 7. backticks around paths in warnings dont seem necessary, maybe just use conventional code treatment or smth
  • 8. the form shouldn't be submitted when pressing enter while editing a key or a value
  • 9. something should be done with "schema id ignored" warnings in the console
  • 10. are we sure we dont need any placeholders? imo the ui looks a little empty and confusing without them, with just the empty white rectangles (placeholder text doesnt get anywhere as the actual data, it's just for display purposes) @akarve
  • 11. value field cant be focused if the relevant key field is empty -- this is a deliberate decision, but seems a little confusing
  • 12. when adding an item to an array there's a react key conflict error in the console
  • 13. consider making breadcrumbs clickable, and maybe adding a root segment to go back to the top
  • 14. pressing escape while a field is focused should not close the dialog
  • 15. there's a strange placeholder / skeleton at the top-left of the packages screen
  • 16. warn on duplicate keys

Code review feedback

  • 1. why do you use i18nMsgs dicts everywhere? doesn't seem like this adds any value / developer convenience. consider removing them and using inline strings instead
  • 2. do not add empty lines everywhere between styles (classes), since brackets and indentation seem enough for making the structure clear and readable
  • 3. there's a possible bug with the anchorRef
  • 4. remove the flag and the old code
  • 5. avoid using lodash when we can use ramda or vanilla js
  • 6. name constants in UPPER_CASE
  • 7. sort imports by module name alphabetically
  • 8. cache compiled schema for perf
  • 9. avoid (re)instantiating objects in function bodies, either move them out or memoize

plus inline comments

Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new changes look mostly good, except for some minor issues

inner: {
display: 'flex',
overflowX: 'auto',
'-ms-overflow-style': 'none',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a note for the future: in JSS this can be written as MsOverflowStyle. the general rule is it inserts a dash before and lowers every uppercase letter.

display: 'flex',
overflowX: 'auto',
'-ms-overflow-style': 'none',
'scrollbar-width': 'none',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, scrollbarWidth would do the same, but w/o quotes

Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is good to go. @akarve pls wordsmith the docs (in a new pr probably)

@fiskus fiskus merged commit 92d2fe8 into master Nov 12, 2020
@fiskus fiskus deleted the json-editor branch November 12, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants