Skip to content

Conversation

@lag-of-death
Copy link

@lag-of-death lag-of-death commented Jul 2, 2019

@lag-of-death lag-of-death changed the title feat: add mask editor (STU-313) JSE : Mask Editor (STU-313) Jul 4, 2019
@P0lip P0lip changed the title JSE : Mask Editor (STU-313) feat: mask editor [STU-313] Jul 4, 2019
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

The UI is not quite right. We should be using the Checkbox component from Blueprint
image

It does not take allOf merging we have into account, therefore schemas containing allOf combiners are likely to be broken.

(node, rowOptions) => {
return (
<SchemaRow
maskControls={() => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done differently, we cannot have it hardcoded like this.
JSV is open-source and therefore likely to be used by more entities and users than just Stoplight.

Choose a reason for hiding this comment

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

@P0lip can you add some suggestions on how do you think this should be done. Will be easier to process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Custom renderRow function. There is a similar case in tree-list and other components.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that we should make JSV more flexible & provide the support for allOf.
By support for allOf, I mean exposing mapped paths, as right now there is no way to determine the path of the original property.

Masking components could be placed in Studio.

Copy link
Author

Choose a reason for hiding this comment

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

@P0lip, so do you mean, that MaskControls component should be placed in Studio (together with its state - [selectedProps, setSelectedProps] and passed to JSV through JSE ? Or do you mean that the whole rowRenderer should be composed in Studio? I'm not sure if the second is an option since it would mean moving SchemaRow component to Studio too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or do you mean that the whole rowRenderer should be composed in Studio?

Yeah, rowRenderer prop should be exposed in JSV and the rest should be composed in Studio.
You can export SchemaRow from JSV, similarly to tree-list and its default row.

Copy link

@chris-miaskowski chris-miaskowski left a comment

Choose a reason for hiding this comment

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

Similarly to JSE: I will skip the review on that for now since I'll be working on extending this with SRN anyway. Plus this will most likely change once Matt is done with the array/allOf support

(node, rowOptions) => <SchemaRow node={node} rowOptions={rowOptions} {...itemData} />,
[itemData.count],
(node, rowOptions) => {
const possibleProps = props.maskControlsHandler
Copy link
Contributor

@P0lip P0lip Jul 5, 2019

Choose a reason for hiding this comment

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

Suggest passing custom rowRenderer rather than having this weird logic here.
You can refer to https://github.com/stoplightio/tree-list/blob/master/src/components/TreeListItem.tsx#L39 to have a better understanding.

@marbemac
Copy link
Contributor

marbemac commented Jul 5, 2019

Agree with @P0lip re keeping mask specific terminology (like maskControls) out of this repo - it should remain fairly generic and be focused on rendering json schemas.

We have a couple of options:

  1. rowRenderer as @P0lip mentioned. However, doing it this way means we have to re-create a lot of the row UI ourselves when used in mask editor (the colors, spacing, etc), which isn't fun.
  2. Add a rowRenderRight function prop that receives the row info, and can render react element(s) that are placed on the right side of the row. Could do same for left side, but I don't think we need that now.

Related to number 2, UX wise I suggest that we render the required toggle and the checkbox on the right side of the rows (required, then checkbox). Putting them on the right side will keep them all aligned, which makes it easier to just move your mouse down the list and click checkboxes as you go. In addition, the required button will end up right next to the required text which better associates the button to the thing it's changing.

@P0lip
Copy link
Contributor

P0lip commented Jul 5, 2019

@marbemac
Speaking of rowRenderer - this shouldn't be as complicated as it may seem to.
We could export a default row component, same as we do in case of tree-list and its row.
This would mean splitting the current Row component into more composable pieces so that you can glue everything together in Studio.
As you said, re-implementing everything in Studio is certainly not the best idea.

@P0lip P0lip force-pushed the STU-313_add_mask_editor branch from 74281d2 to 02d6a9b Compare July 10, 2019 12:31
@chris-miaskowski
Copy link

closing in favour of #37

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.

5 participants