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.

Sign up for GitHub

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

Jump to bottom

fix: update theme generation for new schema #142

Merged
merged 1 commit into from
Oct 26, 2021
Merged

fix: update theme generation for new schema #142

merged 1 commit into from
Oct 26, 2021

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Oct 19, 2021 •
edited
Loading

Limitations with smithy required schema change. Smithy cannot have type unions.

@dpilch dpilch force-pushed the fix-theme branch 5 times, most recently from 55bfc8b to 223a9dd Compare October 20, 2021 14:47
@dpilch
Copy link
Member Author

dpilch commented Oct 21, 2021

Dependent on #152

@dpilch dpilch changed the base branch from main to ui-next October 25, 2021 15:10
@dpilch dpilch force-pushed the fix-theme branch 3 times, most recently from af73f64 to f0fb16b Compare October 25, 2021 15:42
Base automatically changed from ui-next to main October 25, 2021 22:34
@dpilch dpilch force-pushed the fix-theme branch 2 times, most recently from dcb681a to f8a3019 Compare October 26, 2021 15:53
@dpilch dpilch marked this pull request as ready for review October 26, 2021 16:10
alharris-at
alharris-at approved these changes Oct 26, 2021
View reviewed changes
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.

Overall lgtm, one non-blocking nitpick. Moving the hook out simplifies the generated code significantly.

packages/amplify-ui-codegen-schema/lib/types/index.ts Outdated
};

export type StudioThemeValues = {
[String: string]: StudioThemeValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible could you give the key a more descriptive name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm not sure why I put String here...

@dpilch
fix: update theme generation for new schema
4e9b200 
Limitations with smithy required schema change.
@dpilch dpilch requested a review from alharris-at October 26, 2021 16:58
@dpilch dpilch merged commit a780893 into main Oct 26, 2021
@dpilch dpilch deleted the fix-theme branch October 26, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alharris-at alharris-at alharris-at approved these changes

@frimfram frimfram Awaiting requested review from frimfram

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpilch @alharris-at