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

Show JSON values nicely #73

Merged
1 commit merged into from
Jan 3, 2023
Merged

Show JSON values nicely #73

1 commit merged into from
Jan 3, 2023

Conversation

@ghost ghost self-assigned this Dec 15, 2022
@ghost ghost changed the title additional_checks: Remove Column work Dec 15, 2022
@ghost ghost mentioned this pull request Dec 15, 2022
@ghost
Copy link
Author

ghost commented Dec 15, 2022

@lgs85 @duncandewhurst Can you review this one on dev server?

Basically anywhere we show JSON values - JSON schema checks, python checks, additional fields checks - it's going through a new template system. This puts " around strings, makes lists actually be lists and makes long lists hidden by default.

Can you check and make sure this looks good, and there aren't unintended side affects?

(The style of show hide links doesn't look great, but it looks like the other ones we currently have and that's already being dealt with in #65 )

JV1
JV3
JV2
JV4

@duncandewhurst
Copy link
Contributor

Please can you share a link to the dev site?

Looking at the screenshots:

  • Value column is missing from the results for 'value does not match any codes':
    image
  • Value column for node location coordinates should show the raw JSON value, rather than a formatted list:
    image

Otherwise, looks good!

ghost pushed a commit that referenced this pull request Dec 16, 2022
Caught in #73

Introduced in commit 04b6d43
ghost pushed a commit that referenced this pull request Dec 16, 2022
Caught in #73

Introduced in commit 04b6d43
@ghost
Copy link
Author

ghost commented Dec 16, 2022

Please can you share a link to the dev site?

Messaged

Value column is missing from the results for 'value does not match any codes':

Whoops, a one character typo got in - will be fixed here or in another PR

Value column for node location coordinates should show the raw JSON value, rather than a formatted list:

If we are saying there should be exceptions to trying to show JSON values nicely, can we be clear about what they are?

  • eg If Node Location Coordinates is one exception, is Span Location Coordinates another?
  • eg are we sure? A spans location coordinates ending up in a node by mistake could have a very long list, and elsewhere we have said long lists should be hidden by default which makes sense

@ghost ghost changed the title work Show JSON values nicely Dec 16, 2022
@duncandewhurst
Copy link
Contributor

If we are saying there should be exceptions to trying to show JSON values nicely, can we be clear about what they are?

If we're showing a JSON value from the data, then we should show it 'raw', i.e. not formatted as a bulleted list. It would still be good to hide/collapse long values, but not to format them. If we're showing a JSON value from the schema (like an enum) then I think it's OK to format that as a bulleted list. I didn't realise that the list of codes was a JSON value.

* eg If Node Location Coordinates is one exception, is Span  Location Coordinates another?

Yes.

* eg are we sure? A spans location coordinates ending up in a node by mistake could have a very long list, and elsewhere we have said long lists should be hidden by default which makes sense

Hiding it would be good, but not formatting it as a bulleted list.

@ghost ghost force-pushed the 2022-12-15 branch from e42fa2d to ba98bb3 Compare December 20, 2022 08:38
@ghost ghost marked this pull request as draft December 20, 2022 14:49
@ghost
Copy link
Author

ghost commented Dec 20, 2022

I think some discussion would be good. I'm not clear how we would hide/collapse long values without some kind of formatting and if strings should still be shown quoted (I'm guessing yes, but technically showing raw would mean no).

(I suspect this one is going to take more time to sort out than I have left in the sprint, sorry.)

@ghost
Copy link
Author

ghost commented Dec 20, 2022

Try to get this in: Just have quotes around strings, leave lists as raw JSON and don't try and collapse them.

@ghost ghost force-pushed the 2022-12-15 branch from ba98bb3 to a1af12c Compare December 21, 2022 10:00
@ghost ghost marked this pull request as ready for review December 21, 2022 10:03
@ghost
Copy link
Author

ghost commented Dec 21, 2022

@duncandewhurst @lgs85 Please review and if you are happy pass to @Lathrisk for code review, then merge

@duncandewhurst
Copy link
Contributor

Looks good to me, but I think I missed the opportunity for @Lathrisk to review!

@ghost ghost force-pushed the 2022-12-15 branch from a1af12c to 1b37e22 Compare January 3, 2023 10:46
@ghost ghost merged commit d2cb7c2 into live Jan 3, 2023
@ghost ghost deleted the 2022-12-15 branch January 3, 2023 11:28
This pull request was closed.
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