feat: new JSV design #103

mallachari · 2021-02-04T10:06:19Z

Resolves stoplightio/elements#657
New JSE multi-row design replacing ui-kit with mosaic components.

Description and validations are displayed within a row and row height is calculated based on its number.

The naming format of validations is subject of different issue so I just put them the way they are in schema using similar method they are displayed in Elements. Number validations could be moved next to property and type.

Object types are displayed same way they were previously. The final goal is to include a dropdown next to type and allow selecting and displaying proper type but that's out of scope of this issue.

The remaining problem is multiline description and wrapped elements in validations that should make the row grow dynamically depending on their size - though there's separate issue for that.

philsturgeon · 2021-02-04T13:15:00Z

Amazing! So this is solving stoplightio/elements#657 and just happens to do a little bit of stoplightio/elements#667 and stoplightio/elements#668 but is leaving them to be finished separately?

Fantastic, works for me.

mallachari · 2021-02-04T13:21:21Z

Amazing! So this is solving #657 and just happens to do a little bit of #667 and #668 but is leaving them to be finished separately?

It does do part of #668 as those validations have big impact on way of rendering rows.
#667 is separate thing (and quite complex one) so I just mentioned here that handling and displaying multitypes will change, but it's not touched here.

philsturgeon · 2021-02-04T13:32:10Z

Ok great, because an end goal for this epic will be to have no more oneOf, anyOf, or OpenAPI keywords showing here at all.

Keep up the awesome work!

marbemac

Not reviewing the entire thing or anything - but just dropped some comments re canonical usage of Mosaic for the first few files I looked at. We should start removing usage of tailwind classes as much as possible - instead opting for the typed props available on most mosaic components. Of course there are still areas we have to use tailwind classes, that's ok - but eventually we're going to want to remove ui-kit which means no non prefixed tailwind classes.

You can find a list of most of the core style props at https://mosaic.vercel.app/docs/style-props.

src/__stories__/JsonSchemaViewer.tsx

src/components/JsonSchemaViewer.tsx

src/components/SchemaRow.tsx

src/components/shared/Caret.tsx

src/components/shared/Description.tsx

src/components/shared/Divider.tsx

mallachari · 2021-02-05T07:05:52Z

Not reviewing the entire thing or anything - but just dropped some comments re canonical usage of Mosaic for the first few files I looked at. We should start removing usage of tailwind classes as much as possible - instead opting for the typed props available on most mosaic components. Of course there are still areas we have to use tailwind classes, that's ok - but eventually we're going to want to remove ui-kit which means no non prefixed tailwind classes.

You can find a list of most of the core style props at https://mosaic.vercel.app/docs/style-props.

Yup, I'm doing that in my next comit. I just shared first draft so it could be checked.

package.json

src/__stories__/_styles.scss

marcelltoth · 2021-02-08T08:35:22Z

src/components/shared/Caret.tsx

      iconSize={size}
-      icon={isExpanded ? 'caret-down' : 'caret-right'}
-      className="text-darken-9 dark:text-lighten-7"
+      icon={isExpanded ? 'chevron-down' : 'chevron-right'}


The reason I'm worried about this usage of icons is that there's an implicit dependency here that is hidden. We depend on mosaic importing faChevronDown and faChevronRight into its library which is the case right now, but

It may change. Is the list of imported icons part of the mosaic's SEMVER contract? We don't know. It's not documented, so the safe assumption would be that it's not.

If you use strings another developer may - in the future - assume that they are free to change the icons. While in reality they are not. This can lead to hard-to-find bugs.

Type safety is nonexistent, mosaic will happily take 'chevron-dwn', and readability is worse, too, when reading the code you have to go to the docs to figure out what these strings are supposed to be.

I had a discussion about these doubts of mine with @marbemac before, and while he didn't agree completely, we were given the opportunity to use icons in an explicit and type-safe way. If you mark the FA dependency explicitly, then you can import and use the icons:

import {faChevronDown, faChevronRight} from '@fortawesome/free-solid-svg-icons'; ///.... <Icon icon={isExpanded ? faChevronDown : faChevronRight} //...

(Adding the FA dependency doesn't increase the bundle size in this case as mosaic already depends on it, just makes it a direct dep rather than a transient.)

I'm not 100% convinced but I see the point and it's reasonable so I've updated it this way :)

FWIW the mosaic icon component will not accept arbitrary strings:

If going the import {faChevronDown, faChevronRight} from '@fortawesome/free-solid-svg-icons'; route, careful re resulting bundle and tree-shakability. It's very easy to end up with the entire fortawesome iconset, which is huge, in downstream bundles.

Re dropping bundled icons question - we would treat that as a breaking change. And in any case each individual icon is so small there is little incentive to drop icons in the future. A list of the bundled icons is documented in the Icon article - https://mosaic.vercel.app/docs/media/icon (we've added a few since the article was created, I'll get it up to date).

One other note is that in certain cases I hope we can use the (pro only) "regular" icons - for example in the JSV figma design it's using the "regular" font awesome icon for the chevrons on the left rather than the thicker "solid" icon. By passing in the icon name without a specific prefix, it gives us the option to adjust the default to "regular" in our ecosystem, via https://mosaic.vercel.app/docs/media/icon#changing-the-default-icon-style. Of course if an icon is meant to have a specific prefix, such as solid, we should include the prefix so that it is not affected by a change to the default icon style.

Not a huge deal, and ya'll are still free to pass in imported icons, but I wanted to make sure you had all the considerations.

src/components/shared/Description.tsx

src/components/shared/Divider.tsx

src/components/SchemaTree.tsx

src/components/SchemaRow.tsx

src/components/shared/Validations.tsx

mallachari · 2021-02-08T12:47:28Z

@marcelltoth speaking of ui-kit removal, I've noticed that TreeList component is using it as a peer dependency so even though we don't use it anymore we still need to keep that dependency or at least document its requirement.

src/components/shared/__tests__/Property.spec.tsx

marcelltoth

Approved but please check out these comments. You don't need to request another review though unless you're adding some significant changes.

marcelltoth · 2021-02-09T15:23:54Z

src/components/shared/Property.tsx


  return (
    <>
      {schemaNode.subpath.length > 0 && shouldShowPropertyName(schemaNode) && (


Could you assert subpath.length > 0 here instead? I know subpath and schemaNode.subpath are the same, but this is something you need to check for, when looking at this at first I am worried if this is a potential OOB condition

You didn't write this, I see. Well in that case it's fully optional. 😄

src/components/shared/Types.tsx

src/components/shared/Validations.tsx

src/components/SchemaTree.tsx

P0lip · 2021-02-10T09:28:23Z

src/components/shared/Validations.tsx

+import { RegularNode } from '@stoplight/json-schema-tree';
+import { Flex, Text } from '@stoplight/mosaic';
+import { Dictionary, Primitive } from '@stoplight/types';
+import { capitalize, keys, omit, pick, pickBy, uniq } from 'lodash';


keys - any reason we can't just use Object.keys?

Only to be consistent with other lodash functions that we needed to use.

stoplight-bot · 2021-06-24T17:10:05Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: new design

4ebc0f9

mallachari requested review from a team and philsturgeon February 4, 2021 10:06

marcelltoth requested a review from P0lip February 4, 2021 11:00

philsturgeon approved these changes Feb 4, 2021

View reviewed changes

marbemac reviewed Feb 5, 2021

View reviewed changes

feat: move to mosaic

6c7902e

marcelltoth reviewed Feb 8, 2021

View reviewed changes

fix: simplify vlidations and address review

1efbe1c

Jakub Jankowski added 3 commits February 8, 2021 14:25

fix: scrolling and divider

8faa3b7

fix: dark mode

bccb846

fix: tests

0a1f443

marcelltoth reviewed Feb 9, 2021

View reviewed changes

src/components/shared/__tests__/Property.spec.tsx Outdated Show resolved Hide resolved

Jakub Jankowski added 2 commits February 9, 2021 11:28

fix: icon typing

912bab8

fix: linting

ca06c5d

marcelltoth approved these changes Feb 9, 2021

View reviewed changes

test: update

c77f451

mallachari marked this pull request as ready for review February 9, 2021 20:13

mallachari requested a review from a team February 9, 2021 20:13

chore: cleanup

9feccb7

marcelltoth approved these changes Feb 10, 2021

View reviewed changes

mallachari mentioned this pull request Feb 10, 2021

feat: drop expanded prop #101

Merged

P0lip approved these changes Feb 10, 2021

View reviewed changes

feat: use interactive mode

716278a

mallachari merged commit 5c70696 into beta Feb 10, 2021

mallachari deleted the feat/new-design branch February 10, 2021 11:02

philsturgeon mentioned this pull request Jun 21, 2021

Release v4.0.0 stable #152

Closed

stoplight-bot added the released label Jun 24, 2021

feat: new JSV design #103

feat: new JSV design #103

Uh oh!

Conversation

mallachari commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philsturgeon commented Feb 4, 2021 • edited by marcelltoth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mallachari commented Feb 4, 2021

Uh oh!

philsturgeon commented Feb 4, 2021

Uh oh!

marbemac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mallachari commented Feb 5, 2021

Uh oh!

Uh oh!

Uh oh!

marcelltoth Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

mallachari Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

marbemac Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marbemac Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mallachari commented Feb 8, 2021

Uh oh!

Uh oh!

marcelltoth left a comment

Choose a reason for hiding this comment

Uh oh!

marcelltoth Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

marcelltoth Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

P0lip Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

mallachari Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

stoplight-bot commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mallachari commented Feb 4, 2021 •

edited

Loading

philsturgeon commented Feb 4, 2021 •

edited by marcelltoth

Loading

marbemac left a comment •

edited

Loading

marbemac Feb 8, 2021 •

edited

Loading

marbemac Feb 9, 2021 •

edited

Loading