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

[icons] docs: add many tags, word associations to icons metadata #5442

Merged
merged 20 commits into from
Sep 12, 2022

Conversation

gabrieljroth
Copy link
Contributor

Fixes #0000

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Added tags to each icon.

Reviewers should focus on:

Tags were added to the file in the correct manner.

Screenshot

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @gabrieljroth! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Hey @gabrieljroth, I haven't forgotten about this PR, it's just very large and will take a while to review. I've looked through the first 300 lines and this is my feedback so far, but I'm sure I will have more feedback for the rest of the changes. If you want to merge these changes sooner, you can break this down into smaller PRs. Otherwise, you can wait until I'm able to review the whole thing here.

packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
@gabrieljroth
Copy link
Contributor Author

@adidahiya, thanks for reviewing the first chunk of the updates! I removed the quotations, the brackets, and any tag that was redundant with the Icon name. I also removed specific tags as per your comments (except ones where I disagreed per my replies).

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

reviewed up to line 900

packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

reviewed up to line 1600

packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Show resolved Hide resolved
gabrieljroth and others added 9 commits August 1, 2022 13:37
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Great edits! Thanks so much :)

Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

reviewed up to line 2100

packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
Gabe Roth and others added 4 commits August 2, 2022 15:03
First pass at new commit suggestions. Next will review the rest and clean things up on my end.

Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

reviewed from line 2100 to the end

packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
@adidahiya adidahiya changed the title enriched icons.json with hack week icon word association results [icons] docs: add many tags, word associations to icons metadata Sep 12, 2022
Left a few comments but accepted almost all changes!

Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Copy link
Contributor Author

@gabrieljroth gabrieljroth left a comment

Choose a reason for hiding this comment

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

Accepted almost all suggestions, left a few comments.

packages/icons/icons.json Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
packages/icons/icons.json Outdated Show resolved Hide resolved
gabrieljroth and others added 2 commits September 12, 2022 14:28
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
@adidahiya adidahiya merged commit 36991e4 into palantir:develop Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants