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 image path to folder nested under dom-examples #293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@wesgarrison wesgarrison requested a review from a team as a code owner January 31, 2025 15:43
@wesgarrison wesgarrison requested review from bsmth and removed request for a team January 31, 2025 15:43
@@ -309,7 +309,7 @@ window.onload = () => {
// Create a notification with the given title
function createNotification(title) {
// Create and show the notification
const img = '/to-do-notifications/img/icon-128.png';
const img = '/dom-examples/to-do-notifications/img/icon-128.png';
Copy link
Member

Choose a reason for hiding this comment

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

This is good for the GitHub version, but will break on local ones, should we do something like:

const basePath = window.origin.includes("github.io") 
  ? "/dom-examples" 
  : "";

const img = `${basePath}/to-do-notifications/img/icon-128.png`;

If it's on GitHub, add the repo to the base path, otherwise use an empty string?

@bsmth
Copy link
Member

bsmth commented Feb 3, 2025

Thanks for opening this one, I think that's a good fix, there's one comment about running locally versus the preview, but I think we could even ignore my suggestion and add a note that this path should be changed in your local versions to get the icon to show. You can quickly test it with:

let local_notification = new Notification("To do list", {
  body: "Task text",
  icon: "/to-do-notifications/img/icon-128.png",
});

let gh_notification = new Notification("To do list", {
  body: "Task text",
  icon: "/dom-examples/to-do-notifications/img/icon-128.png",
});

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.

2 participants