-
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
feat: output theme file #97
Conversation
@@ -422,3 +424,9 @@ export type StudioComponentStorageBindingProperty = { | |||
bucket: string; | |||
key?: string; | |||
}; | |||
|
|||
type DeepPartial<T> = { |
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.
This is how theme is defined in amplify-ui. See https://github.com/aws-amplify/amplify-ui/blob/main/packages/react/src/theming/types.ts#L3-L7
@@ -0,0 +1,835 @@ | |||
/** |
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.
Copied from amplify-ui for now. We will need to add amplify-ui as a dependency in the future.
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 just add the dependency on amplify-ui now instead of hard-coding more files? I'd rather that we don't need to come back and revisit these tasks down the road if it isn't necessary.
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.
Yes, we can do this now. Before we were still operating under not importing amplify-ui
a18db72
to
d75f1d5
Compare
I'll look into why the amplify-category-studio tests are failing. |
Unit tests on |
script: ScriptKind.TSX, | ||
target: ScriptTarget.ES2015, | ||
module: ModuleKind.ESNext, |
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.
Should this be using the default provided in react-studio-template-renderer-helper
?
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.
Yes, will update.
return this.importCollection.buildImportStatements(); | ||
} | ||
|
||
private buildFunction(): FunctionDeclaration { |
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.
I'd recommend dropping a comment on how this fn was generated (as discussed, using something like https://ts-ast-viewer.com) or adding a section on this to the README so it's a bit clearer how the construction of these packages is working.
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.
I'll work on cleaning this up.
7965c83
to
3330da3
Compare
283b2a1
to
5b48e36
Compare
target: ScriptTarget.ES2015, | ||
module: ModuleKind.ESNext, | ||
}; | ||
protected defaultRenderConfig = defaultRenderConfig; |
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.
Can probably lose this line, and just reference the import on line 66
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.
Fixed
Output
./ui-components/theme.tsx
based on a input theme JSON file.See snapshots for example output.
To test I used create react app with typescript.
I generated a theme with the following JSON input.
And we can see the styles were applied to the button.
Screen Shot 2021-09-27 at 3.45.54 PM.png.pdf
(Only PDF upload was working for me ¯\_(ツ)_/¯ )