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

Add documentation for each icon #139

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Add documentation for each icon #139

merged 3 commits into from
Dec 17, 2020

Conversation

michaelspiss
Copy link
Collaborator

My proposal for a per-icon documentation. It includes the icon's name, style, url and its search terms if available. This is only a proposal, feel free to edit!

@michaelspiss michaelspiss mentioned this pull request Dec 17, 2020
9 tasks
Copy link
Collaborator

@brianegan brianegan left a comment

Choose a reason for hiding this comment

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

Hey, thanks so much for tackling this one! Great work :)

tool/generate_font.dart Show resolved Hide resolved
@michaelspiss
Copy link
Collaborator Author

Do you think the doc comments are comprehensive? Is there anything you would add/change?

@brianegan
Copy link
Collaborator

brianegan commented Dec 17, 2020

Do you think the doc comments are comprehensive?

Yeah, actually I thought adding the search terms was a really nice touch! The only other thing you could consider: Passing the label from the JSON through to generateIconDefinition and using that for the first line.

That would change lines like this:

/// Brands acquisitions-incorporated icon

to this:

/// Brands Acquisitions Incorporated icon

I wouldn't say it's a hard requirement or anything, just a bit easier to read. What do you think?

@michaelspiss
Copy link
Collaborator Author

I agree, this looks much cleaner now

@michaelspiss michaelspiss merged commit aa91506 into master Dec 17, 2020
@michaelspiss michaelspiss deleted the icon-doc branch December 17, 2020 16:09
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