-
Notifications
You must be signed in to change notification settings - Fork 113
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 favicon to kedro-viz documentation #1959
Conversation
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
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.
LGTM!
] | ||
|
||
favicons = [ | ||
"https://kedro.org/images/favicon.ico", |
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.
@Huongg Is it possible to add favicon locally from repo instead of downloading from https://kedro.org/images/favicon.ico
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.
hey @jitu5 it's possible but it means we have to do the same download for framework and kedro-datasets, at the moment 3 repos are using the same image from the link
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.
I feel having it in one place for all the repos is easy for maintenance. Also, where are we actually downloading this favicon, here we are just defining the source. Do you know where is the download code ? or does sphinx do it internally
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.
If we are already following this then its fine. I just asked because if the link somehow gets broken, it will break everywhere, and we won't know about it.
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.
@ravi-kumar-pilla it's just accessing the link, so the proper fix will be to get all of this handled inside kedro-sphinx-theme but it requires a lot of investigation i think. Juanlu has already opened the ticket for it so hopefully we get it fixed soon
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.
Thank you @Huongg!
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.
LGTM 👍 ... thank you @Huongg
Description
Fixes #1724 , as a part of the ticket is to add the favicon to our documentation. Ideally we should do it directly in kedro-sphinx-theme.
However @astrojuanlu pointed out that there's some issue with Sphinx theme don't normally have their own conf.py (issue here), therefore the fix for now will be directly inside kedro-viz doc, and the same for kedro datasets
The favicon is now shown through the doc build https://kedro--1959.org.readthedocs.build/projects/kedro-viz/en/1959/
Checklist
RELEASE.md
file