fix(json-schema-viewer): Description for oneOf/anyOf does not display

[jgreen44]

Addresses #7465

Summary:

When selecting oneOf or anyOf and providing a description, the description fails to show. There are several cases where the description will not show:

1. If there are more than one object
2. If there is an empty string on the object's description

I have provided a video to explain these bugs:

7465.-.2021.11.23.at.4.33.37.PM.-.Bug.Showcase.mov

To Test:

1. Check out the branch and run yarn storybook
2. I have provided a video and all use-cases for these bugs.

7465.-.2021.11.23.at.4.45.32.PM.-.Storybook.Explanation.mp4

Changing one line of code fixed this issue, but I'm unclear as to why schemaNode.annotations.descriptions was used instead of schemaNode.originalFragment.descriptions

Checklist

• The basics
  • I tested these changes manually in my local or dev environment
• Tests
  • Added or updated
  • N/A
• Event Tracking
  • I added event tracking and followed the event tracking guidelines
  • N/A
• Error Reporting
  • I reported errors and followed the error reporting guidelines
  • N/A

[P0lip]

I'm not sure whether we need all of these stories.
Overall, a storybook should be used to demonstrate the functionalities of a component, and not serve us a way to let us reproduce a bug - bugs should be covered by tests

[jgreen44]

@P0lip : I created these bugs in Storybook two-fold:

1. So that others had a way of seeing the bugs in action
2. So that I could debug them.

It was expected that these would be taken out of Storybook after the bug fix. Instead of yalc'ing, is there another method of debugging you can recommend (other than using Storybook)?

[P0lip]

So that others had a way of seeing the bugs in action
It was expected that these would be taken out of Storybook after the bug fix. I

gotcha, sounds good 👍

Instead of yalc'ing, is there another method of debugging you can recommend (other than using Storybook)?

You could write a test for that, we have quite a few of them, and then run debugger from there or perform a console based debugging.

[P0lip]

one should never operate on originalFragment. I believe we should make it private to indicate that.
originalFragment may come in handy in certain situations, but it's certainly not meant to be used this way.

[P0lip]

putting the above comment aside, this code won't work the way user expects.
Consider the following:

{
  "type": "object",
  "properties": {
    "oneOf-two-objects-hidden-description": {
      "description": "Description for oneOf with two objects. (BUG)",
      "oneOf": [
        {
          "title": "oneOf-two-items-first-item",
          "properties": {
            "name": {
              "type": "string"
            },
            "lastName": {
              "type": "string"
            }
          }
        },
        {
           "description": "Woooo for oneOf with two objects. (BUG)",
"title": "oneOf-two-items-second-item",
          "properties": {
            "nameTest2": {
              "type": "string"
            },
            "lastNameTest2": {
              "type": "string"
            }
          }
        }
      ],
      "type": "object"
    }
  }
}

in your case, we'll always display the description placed under oneOf, which is incorrect.

json-schema-tree ensures the above description placed under oneOf (and any other compound keyword like anyOf) is inserted into each dependency of it, so you can rely on it.

  const { defaultExpandedDepth, renderRowAddon, onGoToRef, hideExamples } = useJSVOptionsContext();

  const [isExpanded, setExpanded] = React.useState<boolean>(
    !isMirroredNode(schemaNode) && nestingLevel <= defaultExpandedDepth,
  );

  const { selectedChoice, setSelectedChoice, choices } = useChoices(schemaNode);
  const typeToShow = selectedChoice.type;
  const description = isRegularNode(selectedChoice.type) ? selectedChoice.type.annotations.description : null;

this will do the trick.

If the description is empty, I'd say a user wants to hide it for some reason, and we shouldn't display anything.

[P0lip]

this is how the component looks like if my change is applied

[P0lip]

let's remove these stories too and have a test instead.
I doubt people will actually look at stories if they make a change, so having them won't help us catch a regression.

[jgreen44]

@P0lip : For oneOf-one-object-hidden-description-when-empty and anyOf-one-object-hidden-description-when-empty, the object description isn't created initially (line 55). That key:value pair initially doesn't exist. If the user sets a description for the oneOf or anyOf, then the description is shown (line 51). However, if the user then sets a description for the object (line 55), it overwrites the description shown on line 51. That seems to be expected behavior. However, I feel that if the user then deletes the object description, it should revert to showing the description on line 51. Currently, it keeps a "description" : "" with an empty string on line 55. In doing so, the original description that was set on line 51 is never seen again. This, to me, seems like a bug.

[P0lip]

However, I feel that if the user then deletes the object description, it should revert to showing the description on line 51. Currently, it keeps a "description" : "" with an empty string on line 55. In doing so, the original description that was set on line 51 is never seen again. This, to me, seems like a bug.

Yeah, I agree. That feels wrong. We should change that behavior in JSE in Studio.

[P0lip]

I'm going to do it ^

[P0lip]

👍 just make sure to clean up those stories and we should be good to go.

[jgreen44]

@P0lip : I'm going to remove the Storybook code :)

[P0lip]

Suggested change

import './SchemaRow';

we probably no longer need it do we?

[paulatulis]

@jgreen44 was trying to review this but storybook will not start for me. Can you please make a follow up PR and remove this? TY!

[jgreen44]

@paulatulis : I'm sorry, I'm not sure I understand what you mean by "make a follow up PR and remove this"

[paulatulis]

It looks like you import SchemaRow into this file, but I believe it needs to be removed per @P0lip 's suggested change above. When you check out the main branch and try running yarn storybook, are you able to get it started? Maybe it's just me?

[jgreen44]

Storybook has been removed. I will make another PR to remove ./SchemaRow import

[Nezteb]

(I believe she is referring to the line removal suggestion Jakub made)

[stoplight-bot]

🎉 This PR is included in version 4.3.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀