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

Fix: Adding favicon to Kedro Demo #1509

Merged
merged 21 commits into from
Aug 31, 2023
Merged

Fix: Adding favicon to Kedro Demo #1509

merged 21 commits into from
Aug 31, 2023

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Aug 30, 2023

Description

Resolves #1471

Added favicon on the demo website: https://demo.kedro.org/

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@jitu5 jitu5 added Type: Bug Python Pull requests that update Python code labels Aug 30, 2023
@jitu5 jitu5 self-assigned this Aug 30, 2023
@jitu5
Copy link
Contributor Author

jitu5 commented Aug 30, 2023

@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.

 # Serve the favicon.ico file from the "html" directory
 @app.get('/favicon.ico', include_in_schema=False)
 async def favicon():
     return FileResponse(_HTML_DIR / "favicon.ico")

@rashidakanchwala
Copy link
Contributor

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.

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a 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

@jitu5 jitu5 requested a review from yetudada as a code owner August 31, 2023 08:23
RELEASE.md Outdated Show resolved Hide resolved
Co-authored-by: rashidakanchwala <[email protected]>


class TestFaviconEndpoint:
@pytest.fixture
Copy link
Contributor

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",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a 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!!

@jitu5 jitu5 merged commit 525b3f9 into main Aug 31, 2023
1 check passed
@jitu5 jitu5 deleted the fix/favicon_in_demo branch August 31, 2023 13:23
@rashidakanchwala rashidakanchwala mentioned this pull request Aug 31, 2023
5 tasks
@NeroOkwa NeroOkwa requested review from NeroOkwa and removed request for NeroOkwa August 31, 2023 13:57
ravi-kumar-pilla pushed a commit that referenced this pull request Aug 31, 2023
* 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]>
vladimir-mck pushed a commit that referenced this pull request Sep 6, 2023
* 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]>
ravi-kumar-pilla added a commit that referenced this pull request Sep 11, 2023
* 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]>
vladimir-mck added a commit that referenced this pull request Sep 13, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add favicon to Kedro Demo
3 participants