-
Notifications
You must be signed in to change notification settings - Fork 83
fix: name of route in history #1839
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| // Replace current history state to include title in state (some browsers show | ||
| // history entry title from state). We keep the existing state but add _title. | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems hacky to me, I would avoid changing the history state directly. Do you know which browsers need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting a bug that was getting fixed with this. But its not there anymore... seems like the issue was elsewhere.
I'll remove this , modern browsers don't need this, document.title is enough.
| useRemoveSuccessfulChannelOrder(); | ||
| useNotifyReceivedPayments(); | ||
|
|
||
| // Update document.title and the history entry when the route changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be moved to a separate hook? see the ones two lines above for examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! nice idea!
| // Attempt to derive a title from route handles (crumb) or matched route | ||
| // fallback to app name. Use a small typed helper instead of `any` to | ||
| // satisfy lint rules. | ||
| const getCrumbFromHandle = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can use crumbs. This is for breadcrumbs and does not accurately set the title for a specific page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rolznz , Would you like me to instead use .title from the handle field in routes.tsx? Should I add a title to every route which is just having the crumb field but missing the title field...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be OK, but one downside is it will not be dynamic as far as I can tell.
It might be better to wait until we update to React 19. CC @reneaaron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense...
|
We haven't updated to React 19, but that seems a much cleaner way to set the page title: https://react.dev/blog/2024/12/05/react-19#support-for-metadata-tags |
|
@krrish-sehgal I don't think this will work for app connections or app store apps because they have dynamic titles, right? I think we should wait for React 19 so we can do it properly. |
|
okay , marking it as draft again, then, will wait for further instrcutions on this. |

fixes: #1837
It looks like this now:
How I computed the title
I used useMatches() (React Router v6) to get the active route matches.
From each match I try to call match.handle?.crumb() if present — this returns a string (or array of strings for multi-part crumbs in some routes).
I take the last non-null crumb from the matched route list and set that as document.title. If none found, fallback is "Alby Hub"