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: add page title #108

Merged
merged 9 commits into from
Jul 25, 2024
Merged

fix: add page title #108

merged 9 commits into from
Jul 25, 2024

Conversation

GregoMac1
Copy link
Collaborator

Closes #74

@GregoMac1 GregoMac1 self-assigned this Jul 21, 2024
@GregoMac1 GregoMac1 requested a review from fmaclen July 21, 2024 01:34
@GregoMac1 GregoMac1 changed the title Add page title fix: add page title Jul 21, 2024
Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

This implementation appears to be more complex than what I had in mind.

In #74 I suggested simply adding a new component called <Head> that can accept a title: string[].

Then we include this <Head> component on every +page.svelte where we want a custom title.

For example, in /routes/knowledge/[id]/+page.svelte we could do:

<Head title={[knowledge.name, 'Knowledge']} />

That way you can avoid dealing with $settingsStore and most of the other changes (except the implementation of getSessionTitle() which I think it's a good idea 👍 ).

Will also need to remove <title> from +layout.svelte.

@GregoMac1
Copy link
Collaborator Author

@fmaclen I applied the changes.

Now there's only one thing that I'd like you to help me with.

If you go to the Sessions page, the title will be Sessions • Hollama. If you enter a session, the title will be sessionTitle • Sessions • Hollama.

The problem appears when you go back to the Sessions page by clicking it in the sidebar, because the title remains on sessionTitle • Sessions • Hollama when it should change back to Sessions • Hollama. This behavior also happens with Knowledge.

How would you fix it?

@fmaclen
Copy link
Owner

fmaclen commented Jul 23, 2024

The problem appears when you go back to the Sessions page by clicking it in the sidebar, because the title remains on sessionTitle • Sessions • Hollama

I think we should use the <Head/> component only in files named +page.svelte, not in +layout.svelte.

If that doesn't work, we will need to wrap the component in a logic block {#key foo} which triggers a re-render (of whatever components you include the block) anytime foo changes.

@GregoMac1
Copy link
Collaborator Author

You were right! That was the fix, thank you. I think this is ready to be closed now.

@GregoMac1 GregoMac1 requested a review from fmaclen July 23, 2024 21:01
@GregoMac1
Copy link
Collaborator Author

I've just merged main to this branch and fixed the conflicts. I hope i did it correctly 😅

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

Almost done, left you a couple of minor changes.

src/routes/+layout.svelte Outdated Show resolved Hide resolved
@GregoMac1
Copy link
Collaborator Author

@fmaclen I removed the Head component from src/routes/+layout.svelte, but I added a title tag to app.html because the tab title was showing localhost:5173 for a moment before the Head component was rendered.

Copy link

Deploying hollama with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8b88097
Status: ✅  Deploy successful!
Preview URL: https://ac17e0c0.hollama.pages.dev
Branch Preview URL: https://add-page-title.hollama.pages.dev

View logs

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

This looks good, let's ship it! 🚀

@GregoMac1 GregoMac1 merged commit dbea36b into main Jul 25, 2024
2 checks passed
@GregoMac1 GregoMac1 deleted the add-page-title branch July 25, 2024 18:52
@fmaclen
Copy link
Owner

fmaclen commented Jul 25, 2024

🎉 This PR is included in version 0.7.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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 this pull request may close these issues.

Add relevant <title> tags
2 participants