-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: map prop to children prop for variant #297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments, but overall looks good to me. Could you also add a unit test to verify label doesn't get remapped if it's not in your table?
if (childrenPropMapping !== undefined) { | ||
const propsInOverrides = variant.overrides[overridesKey]; | ||
// only remap if children prop is not defined in this particular overrides section | ||
if (propsInOverrides.children === undefined && propsInOverrides[childrenPropMapping] !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if propsInOverrides[childrenPropMapping] === undefined we probably want to map for consistency sake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: nvm, that was all wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if propsInOverrides[childrenPropMapping] === undefined we probably want to map for consistency sake.
hmmm, childrenPropMapping here is referring to the predefined prop that we want to map to children prop(Ex label). For example, we need to remap label prop to children prop for button component
"overrides": {
"Button": {
"label": "ComponentWithVariantWithMappedChildrenProp",
"fontSize": "15px"
}
}
convert to
"overrides": {
"Button": {
"children": "ComponentWithVariantWithMappedChildrenProp",
"fontSize": "15px"
}
}
if children prop is not defined(propsInOverrides.children) and label prop is defined(propsInOverrides[childrenPropMapping], then we will remap label prop to children prop. So if propsInOverrides[childrenPropMapping] is undefined, then we don need to do anything as the label prop is not there.
|
||
this.component.variants.forEach((variant) => { | ||
// loop through the keys in the dict. Ex. button, button.text | ||
Object.keys(variant.overrides).forEach((overridesKey) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Object.key
you can use Object.entries
since you are accessing the values as well as the keys down below.
0e15464
to
a333e2f
Compare
* Example2: Button | ||
* | ||
*/ | ||
private getComponentTypeFromOverrideKey(overrideKey: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this method to the StudioNode class as a static
method, and ensure we've got unit tests covering it?
a333e2f
to
e432c00
Compare
Codecov Report
@@ Coverage Diff @@
## develop #297 +/- ##
===========================================
+ Coverage 89.45% 89.63% +0.17%
===========================================
Files 36 36
Lines 1223 1244 +21
Branches 273 278 +5
===========================================
+ Hits 1094 1115 +21
Misses 126 126
Partials 3 3
Continue to review full report at Codecov.
|
e432c00
to
1404151
Compare
* fix: map prop to children prop for variant * fix: flip generated tests to jsx Co-authored-by: Alexander Harris <alharris@amazon.com>
* fix: map prop to children prop for variant * fix: flip generated tests to jsx Co-authored-by: Alexander Harris <alharris@amazon.com>
* fix: map prop to children prop for variant * fix: flip generated tests to jsx Co-authored-by: Alexander Harris <alharris@amazon.com>
Issue #, if available:
map prop to children prop for variant
Description of changes:
Revision3
Moved getComponentTypeFromOverrideKey function to under StudioNode class
Revision2
In revision 1, missed to handle a case where the overrideKey in variant can be a nested children component(Flex.Flex[0].Flex[0].Button[0]), added a new function to parse overrideKey and get the corresponding component type
Added complex test using real world payload, the code for the nested children components which have the mapping should flip label prop to children prop, and display different text
Added test to not flip prop to children prop if the component is not defined in the mapping table
Revision1
Currently we are not remapping prop to children prop for variant. This change would remap predefined prop to children prop for variant. It is very similar to what we are doing with property binding. It will go through the variants section, and remap all the predefined prop to children for the specific primitive hierarchy based on this mapping
amplify-codegen-ui/packages/codegen-ui-react/lib/primitive.ts
Line 66 in 868867b
Example:
In memory it will turn into
and output
Since we are already taking care of children props in variant, it will follow the existing process, and display different text.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.