Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: primitive children prop mapping #191

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Nov 3, 2021

Existing Text prop value functionality is preserved for now. All existing Studio apps need to update Text property value to label, then Text prop value functionality can be removed.

label was chosen because it does not have any conflicts with existing primitive props.

  • Create a mapping from a "synthetic" prop to children for primitives that will likely have scalars as children.
  • change Primitives enum to Primitive to follow enum style

See packages/studio-ui-codegen-react/lib/react-component-with-children-renderer.ts for mapping logic.

@dpilch dpilch requested review from frimfram and alharris-at and removed request for frimfram November 3, 2021 18:02
Copy link
Contributor

@alharris-at alharris-at left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but I'd want to confirm that we've never seen that 'value' prop used in a text before we release.

@@ -71,7 +71,7 @@
"position": {
"value": "absolute"
},
"value": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned about this - as far as I understood it these complexTests came from a real export from studio ui - if they're publishing the value then we shouldn't be changing this behavior.

Could you confirm with @frimfram if this has been modified at all from the raw output of CLI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that whenever we are "removing" something in a PR, that PR should contain new tests with "realistic" JSONs that get generated from Studio UI Figma import. That would make sure that we don't just work with assumptions and/or fake jsons that were created outside of Studio UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a breaking change I believe the current UI is not compatible with this. In my opinion this is a change that we should make before re:invent because we won't be able to make breaking changes easily after re:invent. I will check with the UI team to get consensus on the functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

To add an example:

Text and Button would have different approaches to put text as a child.

Text:

{
  "componentType": "Text",
  "properties": {
    "value": {
      "value": "text"
    }
  }
}

<Text>text</Text>

Button:

{
  "componentType": "Button",
  "properties": {
    "children": {
      "value": "text"
    }
  }
}

<Button children="text"></Button>

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, definitely agreed that the change makes sense, and harrison and chris have verbally confirmed that the children thing is being used rather than value, but we should just confirm w/ Rene that we're okay with this change, since he brought it up during backlog refinement yesterday.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, waiting on response from René.

@dpilch dpilch marked this pull request as draft November 5, 2021 20:41
@dpilch dpilch force-pushed the remove-special-text-logic branch from 48bf59a to 8ca61da Compare November 5, 2021 21:39
@dpilch dpilch changed the title feat: remove text value special logic feat: primitive children prop mapping Nov 5, 2021
@dpilch dpilch changed the title feat: primitive children prop mapping [WIP] feat: primitive children prop mapping Nov 5, 2021
@dpilch
Copy link
Member Author

dpilch commented Nov 5, 2021

I've updated this PR with WIP primitive children prop mapping.

@dpilch dpilch force-pushed the remove-special-text-logic branch 4 times, most recently from c1668c0 to 622bf71 Compare November 8, 2021 16:39
@dpilch dpilch changed the title [WIP] feat: primitive children prop mapping feat: primitive children prop mapping Nov 8, 2021
@dpilch dpilch marked this pull request as ready for review November 8, 2021 16:41
@dpilch dpilch force-pushed the remove-special-text-logic branch 2 times, most recently from 5a268a7 to b1ebfbc Compare November 8, 2021 18:51
return Object.values(Primitive).includes(componentType as Primitive);
}

export const PrimitiveChildrenPropMapping: Partial<Record<Primitive, string>> = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Got the ok on this list from René.

@dpilch
Copy link
Member Author

dpilch commented Nov 8, 2021

Looking into the failed integ test now.

Remove special logic for Text
@dpilch dpilch force-pushed the remove-special-text-logic branch from b1ebfbc to 06eb67c Compare November 8, 2021 20:35
@dpilch dpilch merged commit d6cf178 into main Nov 8, 2021
@dpilch dpilch deleted the remove-special-text-logic branch November 8, 2021 22:19
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.

3 participants