Skip to content

Conversation

@marcelltoth
Copy link
Contributor

@marcelltoth marcelltoth self-assigned this Apr 14, 2021
@marcelltoth marcelltoth requested review from a team and billiegoose April 14, 2021 15:53
Copy link
Contributor

@mpodlasin mpodlasin left a comment

Choose a reason for hiding this comment

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

LGTM

@marbemac
Copy link
Contributor

Didn't have time to check root level use cases beyond the one in demo w array - @philsturgeon over to you to try to verify as many root situations as possible (that they are handled well and are easy to understand as end user).

@marcelltoth two small UI things:

  1. root prop name should be same size as other props (it's 11px versus 12px - in general i'm not sure if setting things to text size small so high up in tree, and then overriding to base is best since most of it should be base size, but up to you).
  2. font weight for prop names should be semibold, not bold

Screen Shot 2021-04-15 at 11 26 49 AM

@mpodlasin
Copy link
Contributor

Didn't have time to check root level use cases beyond the one in demo w array - @philsturgeon over to you to try to verify as many root situations as possible (that they are handled well and are easy to understand as end user).

@marcelltoth two small UI things:

1. root prop name should be same size as other props (it's 11px versus 12px - in general i'm not sure if setting things to text size small so high up in tree, and then overriding to base is best since most of it should be base size, but up to you).

2. font weight for prop names should be semibold, not bold
Screen Shot 2021-04-15 at 11 26 49 AM
  1. I changed font sizes
  2. Changed bold to semibold, but in this font the change is not visible, next available weight is medium, but it looks too thin.

@marbemac
Copy link
Contributor

Changed bold to semibold, but in this font the change is not visible, next available weight is medium, but it looks too thin.

That's OK - please leave as semibold, we'll be using Inter in our applications and it is a free font so will document it as the recommended font to pair Elements with.

Copy link

@mallachari mallachari left a comment

Choose a reason for hiding this comment

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

Looks better and works well. I couldn't break it or find any edge case.


type TypeSelectorProps = ReturnType<typeof useChoices>;

export const TypeSelector = ({ choices, setSelectedChoice, selectedChoice }: TypeSelectorProps) => {

Choose a reason for hiding this comment

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

What's the point of this component? It is not used anywhere here from what I can see.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcelltoth some info on this? @mallachari is right, it doesn't seem to be used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. That was an attempt to extract the common selector from SchemaRow and TopLevelSchemaRow but it turned out that the two are different so it didn't make sense to do it in the end. I removed it now, thanks @mallachari for spotting it.

@mpodlasin mpodlasin merged commit 5d3acb6 into beta Apr 22, 2021
@mpodlasin mpodlasin deleted the feat/root-level branch April 22, 2021 08:25
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 4.0.0-beta.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants