feat(docs): bring new JSV into Elements #979
Conversation
sl--ml-9 wrapper is needed to avoid current padding issues with JSV.
| {isJSONSchema(schema) && <SchemaViewer className="mt-6" schema={schema} examples={examples} viewMode="write" />} | ||
| {isJSONSchema(schema) && ( | ||
| <Box ml={-9}> | ||
| <JsonSchemaViewer schema={schema as JSONSchema4} viewMode="write" /> |
There was a problem hiding this comment.
why are we casting here? 🤔
There was a problem hiding this comment.
Also, sometimes we use our SchemaViewer wrapper, sometimes we use JSV directly. What is the logic behind that?
There was a problem hiding this comment.
Let me answer the 2nd question first:
SchemaViewer is a wrapper that renders both the schema and the corresponding examples (if any) in a tab-panel. By the new design we don't need this behavior, we simply want to render the schema. If there are any examples those will be displayed on the side inside the Response Examples panel.
Now the cast: To be honest, I don't know. I simply copied the rendering of JsonSchemaViewer from SchemaViewer to here. That had the cast, this needs the cast as well, as JSV only advertises JSONSchema4 support. Now on why, and how to resolve this cast, we should discuss with @stoplightio/void-crew , thanks for raising that. But I think this should happen separately from this as this is the same thing we've been doing since forever.
There was a problem hiding this comment.
In this case I would think about renaming SchemaViewer somehow. It seems it's purpose is more specific than its name suggests.
There was a problem hiding this comment.
I was thinking about that, but didn't want to expand the scope too much. If you think that's fine, I'll do it now!
| if (contents.length === 0 && !description) return null; | ||
|
|
||
| const schema = contents[chosenContent]?.schema; | ||
| const examples = getExamplesObject(contents[chosenContent]?.examples || []); |
There was a problem hiding this comment.
See above, examples are now being rendered by the Response Examples component on the right.
| <div className="HttpOperation__Parameters"> | ||
| <div className="flex items-center"> | ||
| <div className="font-medium font-mono">{parameter.name}</div> | ||
| <div className={cn('ml-2 text-sm', PropertyTypeColors[type])}>{format ? `${type}<${format}>` : type}</div> |
There was a problem hiding this comment.
I like the minimalistic unicolor look 😎
There was a problem hiding this comment.
Hmm, was that supposed to be changed too? I know it matches jsv types and I like it this way. Just wondering if that's the planned thing.
Also we could make that text muted so it looks the same as in jsv.
There was a problem hiding this comment.
It is all grayscale, see here: https://www.figma.com/file/IkNc59AU2JN5Kc1qbd2AiB/Elements?node-id=157%3A0
So it was planned for in the design. It wasn't planned to be done in this issue (the issues don't quite cover the design changes fully, actually), but as JSV removed the colors it also removed the PropertyTypeColors export so I had to do this otherwise we crash.
re muting: good idea, thanks, I'll fix that!
|
|
||
| return ( | ||
| <VStack spacing={4} divider> | ||
| {sortBy(parameters, ['required', 'name']).map((parameter, index) => { |
There was a problem hiding this comment.
Why we remove sorting by index here?
There was a problem hiding this comment.
Did I? I believe that was simply an unused argument so I simply removed it.
| { | ||
| mediaType: 'application/json', | ||
| schema: { | ||
| type: 'object' as const, |
There was a problem hiding this comment.
What's the point of casting 'object' as const?
There was a problem hiding this comment.
Otherwise the entire thingy is not a valid IHttpOperation object, because JSONSchema4 inside IHttpOperation dictates that type: 'object' | 'string" | ..... You know - unless you put the as const - string literals inside objects get widened to string, and that would give me an error when passing as a prop. You can try it if you wish.
There was a problem hiding this comment.
You make a very good point though. It would be much nicer to read if I simply annotated the type of the data variable instead. Let me see if that is feasible.
There was a problem hiding this comment.
Sure it's possible! Updated 😉
|
Deploy preview for stoplight-elements ready! Built with commit eaaf4c5 |
There was a problem hiding this comment.
I'm so glad that it finally makes it into elements. Lot of work put into it by all of us.
Looks great and seems to work as good as before.
One thing I'm wondering about is that removing the border around standalone schema might make it look better. Do you think it should remain?
# Conflicts: # packages/elements/src/components/Docs/Model/Model.tsx # packages/elements/src/context/Components.tsx
I agree. I checked that one out and it doesn't look great. I think however, that that's something to be tackled separately. It's actually not as trivial to do than the others as the schemas can possibly have examples which we need to display. So IMO it needs product input, and unfortunately it is one of those spots that are not even covered on a high level (at least I haven't found anything on Figma, not to say in an issue) |
4 participants
Resolves #978