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

404 when loading notebook favicons #7076

Closed
jtpio opened this issue Sep 27, 2023 · 6 comments · Fixed by #7109
Closed

404 when loading notebook favicons #7076

jtpio opened this issue Sep 27, 2023 · 6 comments · Fixed by #7109
Assignees
Labels
Milestone

Comments

@jtpio
Copy link
Member

jtpio commented Sep 27, 2023

When using Notebook 7 on Binder (or with a different base url), requesting favicons when executing notebook cells give 404 error:

image

This is likely related to the following logic:

// the favicons are provided by Jupyter Server
const notebookIcon = ' /static/favicons/favicon-notebook.ico';
const busyIcon = ' /static/favicons/favicon-busy-1.ico';

@jtpio jtpio added bug status:Needs Triage Applied to issues that need triage labels Sep 27, 2023
@jtpio jtpio added this to the 7.0.x milestone Sep 27, 2023
@navdeepgill14
Copy link

Hello,

I'm interested in working on this issue. Could you please assign it to me? I'll be happy to take on the task and work towards a resolution.

Thank you!
Navdeep

@RRosio RRosio removed the status:Needs Triage Applied to issues that need triage label Oct 3, 2023
@RRosio
Copy link
Collaborator

RRosio commented Oct 3, 2023

Hi @navdeepgill14 thank you for your interest! If you have any questions or would at any point like to be unassigned please feel free to ping me!

@navdeepgill14
Copy link

Hello everyone,

I'm excited to share that I've successfully resolved this issue! 🎉

Here are the steps I took to address the problem:

  1. Updated the index.ts file to use relative paths for favicon URLs.
  2. Removed the hard-coded absolute paths, allowing for dynamic URL generation.
  3. Thoroughly tested these changes in various scenarios, including local hosting and Binder with different base URLs, and confirmed that the 404 errors no longer occur.

The corresponding pull request has been created and is ready for your review. You can view the details of the changes and test it on your end to ensure that it resolves the issue as expected.

Update index.ts to fix the issue

I kindly request you to review the pull request at your earliest convenience. If it works as intended on your end and resolves the issue, please approve it, and I will proceed to close this issue.

Your feedback and collaboration are greatly appreciated.

Thank you!

Best regards,
Navdeep

@jayeshsingh9767
Copy link
Contributor

jayeshsingh9767 commented Oct 16, 2023

is this issue fixed? if not I will Take this up.

@jtpio
Copy link
Member Author

jtpio commented Oct 17, 2023

Well there was an attempt at it linked in the comment just above: #7090

But the contributor seems to have closed it since. So the issue is not fixed yet.

@navdeepgill14
Copy link

Hi @jayeshsingh9767 and @jtpio,
I want to provide an update on the status of this issue. I initially closed it, but it's not because I've abandoned it. I had to close it temporarily because I was working on another issue, and both of these were interrelated. As a result, I created two PRs on the same branch, and one had to be temporarily closed.

I'm committed to addressing this issue, and I'm happy to reopen the PR and continue working on it. However, I'd also like to extend the invitation to others who may be interested in contributing. Collaboration is always welcome.

I do want to make a note that I'm working on this issue as part of a university assignment, and I have the deadline set to complete this issue by 24th October. So, I might need just one more week to finalize and address the problem.

Thanks for your understanding and patience, and let's make sure we get this issue resolved together!

Kind regards,
Navdeep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants