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: add icons to the set #166

Merged
merged 7 commits into from
Oct 2, 2023
Merged

feat: add icons to the set #166

merged 7 commits into from
Oct 2, 2023

Conversation

kryskirkland
Copy link
Collaborator

@kryskirkland kryskirkland commented Sep 29, 2023

Resolves #25027

  • Adds more icons.

  • Revise to alphabetical order for ease of use (adds alphabetical hashes for searchability)

  • Removes List to avoid naming conflict. It looks like ListOutline and List. If approved, will rename List to ListOutline

@github-actions
Copy link

github-actions bot commented Sep 29, 2023

size-limit report 📦

Path Size
dist/components.cjs.production.min.js 186.51 KB (+4.19% 🔺)
dist/components.esm.js 130.93 KB (+5.87% 🔺)

@mikeldking mikeldking changed the title Add icons from arizeweb feat: add icons to the set Oct 2, 2023
@mikeldking
Copy link
Collaborator

Removes List to avoid naming conflict. It looks like ListOutline and List. If approved, will rename List to ListOutline

Typically not good practice to remove something as it requires a major bump.

@mikeldking mikeldking self-requested a review October 2, 2023 18:15
@kryskirkland
Copy link
Collaborator Author

kryskirkland commented Oct 2, 2023

Removes List to avoid naming conflict. It looks like ListOutline and List. If approved, will rename List to ListOutline

Typically not good practice to remove something as it requires a major bump.

@mikeldking ah - List is not originally in this library, only ListOutline. So instead of remove, I think omit from the migration is a better term. Would the impact still be the same?

@mikeldking
Copy link
Collaborator

Removes List to avoid naming conflict. It looks like ListOutline and List. If approved, will rename List to ListOutline

Typically not good practice to remove something as it requires a major bump.

@mikeldking ah - List is not originally in this library, only ListOutline. So instead of remove, I think omit from the migration is a better term. Would the impact still be the same?

Ah I see - yeah better to name outline vs filled. Let's leave as is. Thanks @kryskirkland

Copy link
Collaborator

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

💅

@mikeldking mikeldking merged commit 75dd66c into main Oct 2, 2023
7 of 8 checks passed
@mikeldking mikeldking deleted the icons-from-arizeweb branch October 2, 2023 19:21
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants