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

Add svg logo resource for ipython #1851

Merged
merged 8 commits into from
Sep 16, 2022
Merged

Add svg logo resource for ipython #1851

merged 8 commits into from
Sep 16, 2022

Conversation

AhdraMeraliQB
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB commented Sep 15, 2022

Description

Jupyterlab now prioritises a logo-svg.svg file above .png files in their launcher (see: this issue on jupyterlab). Without including this file, the Kedro logo is replaced by the Python logo that is automatcally included by ipykernel as of 6.15.3. This PR adds this file in order to prevent the Kedro logo from being replaced.

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 a description of this change in the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
@AhdraMeraliQB
Copy link
Contributor Author

Closed accidentally 😅

@AhdraMeraliQB AhdraMeraliQB reopened this Sep 15, 2022
Ahdra Merali added 3 commits September 15, 2022 17:12
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
@AhdraMeraliQB AhdraMeraliQB marked this pull request as ready for review September 15, 2022 16:49
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Copy link
Member

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

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

LGTM!

tests/framework/cli/test_jupyter.py Outdated Show resolved Hide resolved
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this! A couple of quick checks:

  • if you do make package, is the svg definitely included in the .whl file? (I'm very confident it is, owing to the change to MANIFEST.in, but it's worth being absolutely sure because otherwise when you pip install kedro from a released package the kedro jupyter commands would not work)
  • a quick screenshot of our beautiful high fidelity zoomed in logo on Jupyter lab would be nice 🙂 I wonder if we should crop the svg (still keeping it square) since at a glance it seems like it might have quite a big empty space round it?

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Copy link
Contributor

@jmholzer jmholzer left a comment

Choose a reason for hiding this comment

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

This all looks good and proper to me 😃.

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
@AhdraMeraliQB
Copy link
Contributor Author

AhdraMeraliQB commented Sep 16, 2022

@AntonyMilneQB

  • if you do make package, is the svg definitely included in the .whl file? (I'm very confident it is, owing to the change to MANIFEST.in, but it's worth being absolutely sure because otherwise when you pip install kedro from a released package the kedro jupyter commands would not work)

It is! 🎊

  • a quick screenshot of our beautiful high fidelity zoomed in logo on Jupyter lab would be nice 🙂 I wonder if we should crop the svg (still keeping it square) since at a glance it seems like it might have quite a big empty space round it?

Here's a look at the logo at 400% zoom, sharp as ever:

Screenshot 2022-09-16 at 11 21 27

Cropped the logo as suggested to get rid of some (but not all) of the whitespace, let me know if it needs more of a chop ✂️

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
@AhdraMeraliQB AhdraMeraliQB self-assigned this Sep 16, 2022
@antonymilne
Copy link
Contributor

Looks perfect, thank you!

@AhdraMeraliQB AhdraMeraliQB merged commit ab83926 into main Sep 16, 2022
@AhdraMeraliQB AhdraMeraliQB deleted the fix/add-svg-logo branch September 16, 2022 10:56
nickolasrm pushed a commit to ProjetaAi/kedro that referenced this pull request Oct 26, 2022
* Add resource

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Include file in kernel

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Lint

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Fix tests

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Lint (again)

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Alphabetise resources

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

* Crop logo svg

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Crop logo svg more

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: nickolasrm <nickolasrochamachado@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants