Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Docs] Replace PNG/ICO images with SVG #2869
[Docs] Replace PNG/ICO images with SVG #2869
Changes from all commits
900d613
b19cdba
0c7a5b9
7ec2d78
e18db4b
4b980ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe keep it pointing at SVG as a fallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we agree in a good file structure, I can test this.
I suppose it will work fine, although it might obfuscate the setup for multiple sizes in some browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call the file properly instead of having a comment only in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @webknjaz, I am not sure if I understood correctly your comment.
Do you mean having a
"relative_path"
instead of"file"
in the dictionary? That would make sense...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm talking about the filename. It's not cool that the file is called
favicon.svg
and needs a code comment above explaining what it is. This information should be present in a file name, treat it as a variable in code. If the name is self-explanatory, then it'd be useful when checking out the HTML output, or just opening the URL in a browser, or while browsing the repository. A name likefavicon
does not give any hint about what the difference is from the other SVG file, nor it points at the relation between this andlogo-symbol-only.svg
.You've added two code comments for each entry. The first one is quite useful because it explains the motivation but this second one has both useful and pointless bits. If a code comment has to include explanations about what the things are, you're probably calling those things wrong names.