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

Update index.ts to fix the issue #7090

Closed
wants to merge 0 commits into from

Conversation

navdeepgill14
Copy link

Pull Request: Fixing 404 when loading notebook favicons

Issue Description

This pull request addresses the issue of 404 errors occurring when requesting favicons while executing notebook cells in Notebook 7, particularly when using Binder or a custom base URL. The problem originates from the hard-coded favicon URLs in the index.ts file.

Proposed Changes

To resolve this issue, I have updated the favicon URLs in the index.ts file to be relative to the root of the application rather than hard-coded absolute paths. This change ensures that the correct favicon paths are generated, regardless of the base URL configuration.

How This Fixes the Issue

By making the favicon URLs relative, they will adapt to the current application's base URL dynamically. This means that whether the Notebook is hosted on Binder or uses a custom base URL, the favicons will be correctly served without resulting in 404 errors.

Changes Made

  • Modified index.ts to use relative paths for favicon URLs.
  • Removed hard-coded absolute paths.

Testing

I have tested these changes in the following scenarios:

  1. Locally hosted Notebook 7.
  2. Notebook 7 on Binder with various base URLs.
  3. Notebook 7 with a custom base URL configuration.

In each scenario, the favicons were successfully loaded without any 404 errors.

Screenshots

Screen Shot 2023-10-05 at 1 30 54 pm

Checklist

  • Updated index.ts with relative favicon URLs.
  • Removed hard-coded absolute paths.
  • Tested changes in various scenarios.
  • Added appropriate documentation/comments.
  • Screenshots for after.
  • Reviewed and addressed any linting/formatting issues.

Related Issue

This pull request fixes issue #[7076].

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Binder 👈 Launch a Binder on branch navdeepgill14/notebook/main

@jtpio
Copy link
Member

jtpio commented Oct 5, 2023

Thanks @navdeepgill14 for looking into this 👍

Notebook 7 on Binder with various base URLs.

Looks like this is still an issue on Binder? This is what it looks like when trying the Binder for this PR: https://mybinder.org/v2/gh/navdeepgill14/notebook/main?urlpath=tree

image

@jtpio jtpio added the bug label Oct 5, 2023
@@ -411,8 +411,9 @@ const tabIcon: JupyterFrontEndPlugin<void> = {
requires: [INotebookTracker],
activate: (app: JupyterFrontEnd, tracker: INotebookTracker) => {
// the favicons are provided by Jupyter Server
const notebookIcon = ' /static/favicons/favicon-notebook.ico';
const busyIcon = ' /static/favicons/favicon-busy-1.ico';
const baseURL = window.location.origin;
Copy link
Member

@jtpio jtpio Oct 5, 2023

Choose a reason for hiding this comment

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

Maybe this should be PageConfig.getBaseUrl() instead?

@jtpio jtpio added this to the 7.0.x milestone Oct 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants