-
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
Fix: Adding favicon to Kedro Demo #1509
Conversation
@rashidakanchwala @ravi-kumar-pilla Below approach also works by creating separate end point for favicon. Please let me know which one fits as per our best practices.
|
Actually, I like both these approaches. I did find the second approach of creating an endpoint as the most common way of serving favicon on FastAPi when I did some research yesterday. |
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.
Tested on GitPod and looks good to me. Can you please update the release doc before merging ? Thank you
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
…dro-viz into fix/favicon_in_demo # Conflicts: # package/tests/test_api/test_apps.py
Co-authored-by: rashidakanchwala <[email protected]>
package/tests/test_api/test_apps.py
Outdated
|
||
|
||
class TestFaviconEndpoint: | ||
@pytest.fixture |
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.
We don't need this pytest.fixture. We can use client which is defined in conftest.py and just do this directly
def test_favicon_endpoint(self, client):
response = client.get("/favicon.ico")
assert response.status_code == 200
assert response.headers["content-type"] in [
"image/x-icon",
"image/vnd.microsoft.icon",
]
Look at line 11,12 in the same file. And let me know if you have more questions.
assert response.status_code == 200 | ||
assert response.headers["content-type"] in [ | ||
"image/x-icon", | ||
"image/vnd.microsoft.icon", |
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.
QQ - we have 2 content-types here, is it because of linux and windows tests?
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.
Here 'image/x-icon' is commonly used MIME type for ICO files, some server configurations might use 'image/vnd.microsoft.icon' as ICO files content type, in fact our test setup uses 'image/vnd.microsoft.icon' as ICO files content type. So added both content type for checks.
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! and congrats on your first python PR!!
* Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya <[email protected]> * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fixed Signed-off-by: Jitendra Gundaniya <[email protected]> * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <[email protected]> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]> Signed-off-by: ravi-kumar-pilla <[email protected]>
* Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya <[email protected]> * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fixed Signed-off-by: Jitendra Gundaniya <[email protected]> * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <[email protected]> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]> Signed-off-by: Vladimir <[email protected]>
* initial draft for resolving connection error * refactor launchers and test code * modify unit tests * fix lint errors * fix run_viz tests * update unit test for coverage * update unit tests * Refactor visualize dataset stats from DataNodeMetadata to DataNode (#1499) * add stats to data node * lint and format check fix * fix pytests * fix layout issue * fix transcoded data stats Signed-off-by: ravi-kumar-pilla <[email protected]> * initial draft for resolving connection error Signed-off-by: ravi-kumar-pilla <[email protected]> * Support for Python 3.11 (#1502) * initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove print statements --------- Co-authored-by: Nok Lam Chan <[email protected]> Signed-off-by: ravi-kumar-pilla <[email protected]> * Remove Python Upper Bound Requirements (#1506) * initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove python upperbounds initial draft * fix lint and format errors * test remove upperbound warning * test lowerbound pandas install * revert back pandas requirement * bump lower requirements for pandas * remove upper bound clean up * update release notes * fix PR comments --------- Co-authored-by: Nok Lam Chan <[email protected]> Signed-off-by: ravi-kumar-pilla <[email protected]> * refactor launchers and test code Signed-off-by: ravi-kumar-pilla <[email protected]> * modify unit tests Signed-off-by: ravi-kumar-pilla <[email protected]> * fix lint errors Signed-off-by: ravi-kumar-pilla <[email protected]> * Fix: Adding favicon to Kedro Demo (#1509) * Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya <[email protected]> * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fixed Signed-off-by: Jitendra Gundaniya <[email protected]> * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <[email protected]> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]> Signed-off-by: ravi-kumar-pilla <[email protected]> * fix run_viz tests Signed-off-by: ravi-kumar-pilla <[email protected]> * update unit test for coverage Signed-off-by: ravi-kumar-pilla <[email protected]> * Release v6.5.0 (#1513) * v6.5.0 * release * update-reminder-content * update reminder Signed-off-by: ravi-kumar-pilla <[email protected]> * remove branch condition for automate release version check (#1514) Signed-off-by: ravi-kumar-pilla <[email protected]> * update unit tests Signed-off-by: ravi-kumar-pilla <[email protected]> * add release record * modify comment * fix PR comments * DCO fix * fixing dco Signed-off-by: ravi-kumar-pilla <[email protected]> * update pytest Signed-off-by: ravi-kumar-pilla <[email protected]> --------- Signed-off-by: ravi-kumar-pilla <[email protected]> Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: Rashida Kanchwala <[email protected]> Co-authored-by: Nok Lam Chan <[email protected]> Co-authored-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]>
…nto a single css file (#1510) * Update dependencies, update apollo config, update worker to use a mock worker for SSR Signed-off-by: Vladimir <[email protected]> * Fix: Adding favicon to Kedro Demo (#1509) * Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya <[email protected]> * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fix Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <[email protected]> * Lint error fixed Signed-off-by: Jitendra Gundaniya <[email protected]> * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <[email protected]> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]> Signed-off-by: Vladimir <[email protected]> * Release v6.5.0 (#1513) * v6.5.0 * release * update-reminder-content * update reminder Signed-off-by: Vladimir <[email protected]> * remove branch condition for automate release version check (#1514) Signed-off-by: Vladimir <[email protected]> * Update RELEASE.md Signed-off-by: Vladimir <[email protected]> * Update css imports to scss, combine css files into 1 for the npm lib, update babel to remove scss imports from the lib components. Revert apollo config and worker Signed-off-by: Vladimir <[email protected]> * Update package lock Signed-off-by: Vladimir <[email protected]> * Update readme and release me Signed-off-by: Vladimir <[email protected]> * Remove build:css script from circle ci config Signed-off-by: Vladimir <[email protected]> * Bundle css into lib/styles folder Signed-off-by: Vladimir <[email protected]> * Update readme Signed-off-by: Vladimir <[email protected]> * Add webpack config to extract scss and babel plugin to remove scss imports Signed-off-by: Vladimir <[email protected]> * Update RELEASE.md Co-authored-by: Tynan DeBold <[email protected]> * Update RELEASE.md --------- Signed-off-by: Vladimir <[email protected]> Signed-off-by: Jitendra Gundaniya <[email protected]> Co-authored-by: Jitendra Gundaniya <[email protected]> Co-authored-by: rashidakanchwala <[email protected]> Co-authored-by: Ravi Kumar Pilla <[email protected]> Co-authored-by: Tynan DeBold <[email protected]>
Description
Resolves #1471
Added favicon on the demo website: https://demo.kedro.org/
Checklist
RELEASE.md
file