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

feat: add built-in iconset #219

Merged
merged 1 commit into from
Nov 16, 2021
Merged

feat: add built-in iconset #219

merged 1 commit into from
Nov 16, 2021

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Nov 12, 2021 •
edited
Loading

This change uses a version of amplify-ui that is from 11/12. Do not merge until version has propagated to npmpm (~ 11/15)

Add support for built-in iconset codegen. https://ui.docs.amplify.aws/components/icon?platform=react#built-in-iconset

This change will fail with the following error until an update is made to Amplify UI.

node_modules/@aws-amplify/ui-react/dist/esm/index.js:1

// lots of text
                                              ^^^^^^

SyntaxError: Cannot use import statement outside a module

I determined this was because the ESM version of the Amplify UI distribution was being loaded rather than the CommonJS version.

https://github.com/aws-amplify/amplify-ui/blob/main/packages/react/package.json#L4-L10

Changing the exports from the link above to the following will allow use to force use the CommonJS version with a require import.

".": {                                                                                                                
  "import": "./dist/esm/index.js",                                                                                    
  "require": "./dist/index.js"                                                                                        
},

@dpilch dpilch requested a review from alharris-at November 12, 2021 16:38
@dpilch
Copy link
Member Author

dpilch commented Nov 12, 2021

Amplify UI Change: aws-amplify/amplify-ui#696 (review)

ericclemmons
ericclemmons reviewed Nov 12, 2021
View reviewed changes
packages/studio-ui-codegen-react/lib/primitive.ts Outdated
@@ -56,6 +58,8 @@ enum Primitive {

export default Primitive;

export const iconset = Object.keys(AmplifyUI).filter((name) => name.match(/^Icon\w/));
Copy link

Choose a reason for hiding this comment

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

Warning! I've recommended we remove icons (https://github.com/aws-amplify/amplify-ui/discussions/480#discussioncomment-1438384) and instead have 1st class support for 3rd party icon libraries like @heroicons/react, react-icons, etc.

This has been a long-time coming, but this change hasn't landed.

CC @dbanksdesign

Copy link
Contributor

@alharris-at alharris-at Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From a feature complete perspective for ui codegen we need the ui API locked down today, with no breaking changes coming down the pipe. Is it possible to release icons under a new major version (3.x.x) once that's been agreed and implemented?

@hvergara for viz.

alharris-at
alharris-at approved these changes Nov 12, 2021
View reviewed changes
packages/studio-ui-codegen-react/lib/react-studio-template-renderer.ts Outdated
@@ -318,7 +318,7 @@ export abstract class ReactStudioTemplateRenderer extends StudioTemplateRenderer
const propsType = this.getPropsTypeName(component);

const componentIsPrimitive = isPrimitive(component.componentType);
if (componentIsPrimitive) {
if (componentIsPrimitive || iconset.includes(component.componentType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you use it in a few place, could you just extract this fn to like 'componentIsIcon'

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

@dpilch
Copy link
Member Author

dpilch commented Nov 12, 2021

Added isBuiltInIcon and changed icon list to Set for slight performance benefit.

@dpilch dpilch changed the title feat: add built-in iconset [DO NOT MERGE] feat: add built-in iconset Nov 12, 2021
@dpilch
Copy link
Member Author

dpilch commented Nov 15, 2021

Dependent on aws-amplify/amplify-ui#709

@dpilch dpilch changed the title [DO NOT MERGE] feat: add built-in iconset feat: add built-in iconset Nov 16, 2021
@dpilch dpilch force-pushed the builtin-iconset branch 4 times, most recently from 8c801e2 to d5363f8 Compare November 16, 2021 20:34
@dpilch dpilch marked this pull request as ready for review November 16, 2021 20:42
@dpilch dpilch merged commit d3e097b into main Nov 16, 2021
@dpilch dpilch deleted the builtin-iconset branch November 16, 2021 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericclemmons ericclemmons ericclemmons left review comments

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

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

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