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

Make iconSlug explicit for all products #2087

Merged
merged 1 commit into from
Dec 20, 2022
Merged

Make iconSlug explicit for all products #2087

merged 1 commit into from
Dec 20, 2022

Conversation

marcwrobel
Copy link
Member

Being explicit is simpler (especially on API side, see #759) and causes less confusion.

@marcwrobel marcwrobel linked an issue Dec 18, 2022 that may be closed by this pull request
Make iconSlug explicit for all products #2085
Closed
captn3m0
captn3m0 reviewed Dec 19, 2022
View reviewed changes
Copy link
Member

@captn3m0 captn3m0 left a comment

Choose a reason for hiding this comment

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

Please update the template to remove the permalink version to make it explicit on the HTML side as well.

@marcwrobel
Make iconSlug explicit for all products (closes #2085)
ea0648d 
Being explicit is simpler (especially on API side, see #759) and causes less confusion.
@marcwrobel marcwrobel force-pushed the 2085-explicit-iconslug branch from 555a0a0 to ea0648d Compare December 19, 2022 11:44
@marcwrobel
Copy link
Member Author

Please update the template to remove the permalink version to make it explicit on the HTML side as well.

Done.

@marcwrobel marcwrobel requested a review from captn3m0 December 19, 2022 11:45
@captn3m0
Copy link
Member

captn3m0 commented Dec 19, 2022 via email

@marcwrobel
Copy link
Member Author

marcwrobel commented Dec 19, 2022 •
edited
Loading

We should do a null check instead of NA. Will need changes to other products as well. Here or second PR?

Second PR would be simpler (but let's leave #2085 open).

By null do you mean the attribute must be removed from product having iconSlug=NA, or the attribute must be set to null ?

@captn3m0
Copy link
Member

captn3m0 commented Dec 20, 2022 •
edited
Loading

Must be removed, so our docs can go from:

If the icon is not available on Simple Icons, set it to NA.

to

Only set an iconSlug if an icon is available on Simple Icons

captn3m0
captn3m0 approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@captn3m0 captn3m0 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's get rid of iconSlug: NA hack in a second PR.

@captn3m0 captn3m0 merged commit 60e7f9c into master Dec 20, 2022
@captn3m0 captn3m0 deleted the 2085-explicit-iconslug branch December 20, 2022 06:14
@marcwrobel marcwrobel mentioned this pull request Dec 20, 2022
Closed
@marcwrobel marcwrobel added the enhancement New feature or request label Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captn3m0 captn3m0 captn3m0 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make iconSlug explicit for all products
2 participants
@marcwrobel @captn3m0