Skip to content

Breaking Change: Remove built-in icons #2082

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

Merged
merged 22 commits into from
Jun 13, 2022
Merged

Breaking Change: Remove built-in icons #2082

merged 22 commits into from
Jun 13, 2022

Conversation

reesscot
Copy link
Contributor

@reesscot reesscot commented Jun 11, 2022

Description of changes

This PR is an updated version of #1310. It removes all the icons not used in our code base and updates all the docs examples to use pathData instead of the built-in icons.

@aws-amplify/ui-react BEFORE removing icons:

npm notice === Tarball Details === 
npm notice name:          @aws-amplify/ui-react                   
npm notice version:       2.19.1                                  
npm notice filename:      @aws-amplify/ui-react-2.19.1.tgz        
npm notice package size:  773.5 kB                                
npm notice unpacked size: 6.2 MB                                  
npm notice shasum:        ac9a71590f1750b68f6b889a88b1b0a3347638e5
npm notice integrity:     sha512-MLYaghCnYK7Cv[...]rim+eNwHvwyKw==
npm notice total files:   3091                                    
npm notice 

@aws-amplify/ui-react AFTER removing icons:

npm notice === Tarball Details === 
npm notice name:          @aws-amplify/ui-react                   
npm notice version:       2.19.1                                  
npm notice filename:      @aws-amplify/ui-react-2.19.1.tgz        
npm notice package size:  195.0 kB                                
npm notice unpacked size: 2.1 MB                                  
npm notice shasum:        a88772137083cfaf30cc7cefc098847365643e4d
npm notice integrity:     sha512-zo0DYkzZCrLJh[...]7BsrC8c75Frww==
npm notice total files:   469                                     
npm notice 

Issue #, if available

Description of how you validated changes

Checklist

  • Add BREAKING CHANGE changeset
  • PR description included
  • yarn test passes
  • Tests are updated
  • No side effects or sideEffects field updated
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@changeset-bot
Copy link

changeset-bot bot commented Jun 11, 2022

🦋 Changeset detected

Latest commit: 537cd03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/ui-react Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@reesscot reesscot marked this pull request as ready for review June 12, 2022 00:01
@reesscot reesscot changed the title Remove icons Breaking Change: Remove icons Jun 12, 2022
@reesscot reesscot changed the title Breaking Change: Remove icons Breaking Change: Remove built-in icons Jun 12, 2022
@reesscot reesscot temporarily deployed to ci June 12, 2022 00:11 Inactive
Copy link
Contributor

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM!

@ayush987goyal
Copy link
Contributor

Can you please make sure the following icons are not removed since they are being used in the staging repo?

  1. IconClose
  2. IconCheckCircleOutline
  3. IconHighlightOff
  4. IconFiberManualRecord

@reesscot
Copy link
Contributor Author

  • IconCheckCircleOutline

  • IconHighlightOff

  • IconFiberManualRecord

Thanks for the review @ayush987goyal. IconClose is still there but I will add the other three back:
IconCheckCircleOutline
IconHighlightOff
IconFiberManualRecord

@reesscot
Copy link
Contributor Author

  • IconCheckCircleOutline
  • IconHighlightOff
  • IconFiberManualRecord

Thanks for the review @ayush987goyal. IconClose is still there but I will add the other three back: IconCheckCircleOutline IconHighlightOff IconFiberManualRecord

Added back the icons needed by staging repo

@lgtm-com
Copy link

lgtm-com bot commented Jun 12, 2022

This pull request introduces 1 alert when merging d476820 into 28bf92d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ayush987goyal
Copy link
Contributor

ayush987goyal commented Jun 12, 2022

Thanks @reesscot . Let's discuss offline to see how can we remove the dependency from staging repo on these icons. We can then remove these ones too later.

@reesscot reesscot temporarily deployed to ci June 13, 2022 17:44 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 17:44 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 17:44 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 18:14 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 18:14 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 18:14 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 18:14 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 19:09 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 19:09 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 19:09 Inactive
@reesscot reesscot temporarily deployed to ci June 13, 2022 19:09 Inactive
@reesscot reesscot merged commit e79f418 into main Jun 13, 2022
@reesscot reesscot deleted the remove-icons branch June 13, 2022 20:36
@github-actions github-actions bot mentioned this pull request Jun 13, 2022
@reesscot reesscot mentioned this pull request Jun 13, 2022
3 tasks
Comment on lines +1 to +3
---
"@aws-amplify/ui-react": major
---
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

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.

5 participants