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: output theme file #97

Merged
merged 7 commits into from
Oct 11, 2021
Merged

feat: output theme file #97

merged 7 commits into from
Oct 11, 2021

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Sep 27, 2021

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.

import React from 'react';
import logo from './logo.svg';
import './App.css';
import withTheme from './ui-components/theme';  // add theme provider
import { Button } from '@aws-amplify/ui-react'; 
import '@aws-amplify/ui-react/styles.css'; // add default themes

function App() {
 return (
    <div className="App">
      <header className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
        <p>
          Edit <code>src/App.tsx</code> and save to reload.
        </p>
        <a
          className="App-link"
          href="https://reactjs.org"
          target="_blank"
          rel="noopener noreferrer"
        >
          Learn React
        </a>
      </header>
      <Button >Test</Button>
    </div>
  );
}

export default withTheme(App); // wrap App with theme
```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.

```js
import React from 'react';
import logo from './logo.svg';
import './App.css';
import withTheme from './ui-components/theme';  // add theme provider
import { Button } from '@aws-amplify/ui-react'; 
import '@aws-amplify/ui-react/styles.css'; // add default themes

function App() {
 return (
    <div className="App">
      <header className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
        <p>
          Edit <code>src/App.tsx</code> and save to reload.
        </p>
        <a
          className="App-link"
          href="https://reactjs.org"
          target="_blank"
          rel="noopener noreferrer"
        >
          Learn React
        </a>
      </header>
      <Button >Test</Button>
    </div>
  );
}

export default withTheme(App); // wrap App with theme

I generated a theme with the following JSON input.

{ "components": { "button": { "borderWidth": "40px" } } }

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 ¯\_(ツ)_/¯ )

@@ -422,3 +424,9 @@ export type StudioComponentStorageBindingProperty = {
bucket: string;
key?: string;
};

type DeepPartial<T> = {
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,835 @@
/**
Copy link
Member Author

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.

Copy link
Contributor

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.

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, we can do this now. Before we were still operating under not importing amplify-ui

@dpilch dpilch marked this pull request as ready for review September 27, 2021 16:46
@dpilch dpilch force-pushed the theming branch 2 times, most recently from a18db72 to d75f1d5 Compare September 27, 2021 21:38
@dpilch
Copy link
Member Author

dpilch commented Sep 27, 2021

I'll look into why the amplify-category-studio tests are failing.

@dpilch
Copy link
Member Author

dpilch commented Sep 28, 2021

Unit tests on amplify-category-study are passing now. It was due to an issue with amplify-category-studio dependencies: https://github.com/johnpc/amplify-category-studio/commit/698be368874958063cd12ab007d674c15b607ab7

Comment on lines 52 to 54
script: ScriptKind.TSX,
target: ScriptTarget.ES2015,
module: ModuleKind.ESNext,
Copy link
Contributor

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?

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, will update.

return this.importCollection.buildImportStatements();
}

private buildFunction(): FunctionDeclaration {
Copy link
Contributor

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.

Copy link
Member Author

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.

@dpilch dpilch force-pushed the theming branch 4 times, most recently from 7965c83 to 3330da3 Compare October 4, 2021 19:37
@dpilch dpilch marked this pull request as draft October 6, 2021 21:00
@dpilch dpilch force-pushed the theming branch 4 times, most recently from 283b2a1 to 5b48e36 Compare October 7, 2021 21:02
@dpilch dpilch marked this pull request as ready for review October 7, 2021 21:04
target: ScriptTarget.ES2015,
module: ModuleKind.ESNext,
};
protected defaultRenderConfig = defaultRenderConfig;
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@dpilch dpilch merged commit 02508c1 into main Oct 11, 2021
@dpilch dpilch deleted the theming branch October 11, 2021 14:31
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.

2 participants