-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Feature: Update window title on page change #6119
Feature: Update window title on page change #6119
Conversation
@djhi I can't get my head around moving the tip of a branch (e.g. moving the last commit from the tip of master over to next), so I just made a new pull request If you have a git helper doc to hand for the above issue, i'd be eternally grateful! |
Morning folks! I'm wondering if there are any other changes you'd like me to make for this pull request. @fzaninotto @djhi |
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.
Awesome!
This feature needs basic documentatoin, e.g. in https://marmelab.com/react-admin/CreateEdit.html#page-title, where you should explain that, when using an element as title, you have to set the document title by hand - and show an example usage of the hook.
Thanks for all the feedback and suggestions, @fzaninotto , I've pushed the fixes and resolved them all. I'll behappy to make any other changes as needed Edit: Just realised I missed one of the changes, I'll get this updated |
Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com>
Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com>
….com:andrico1234/react-admin into feature/update-window-title-on-page-change-2
Thanks for the re-review @fzaninotto , I've implemented the requested changes! |
Hi @fzaninotto , are there any other changes you'd like? |
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 saw some more adjustments to make, I should've seen them during y first review, sorry.
Hi @fzaninotto , I've made the required changes. Is there anything else I should change before hand? |
Thanks! |
We haven't noticed, but this is a breaking change. If a developer has set the document title to "Acme" globally, it's replaced by the page title (e.g. "Posts"), without any simple way to insert the main app name globally. So I'm going to revert this PR as is must provide an additional feature without removing the current abilities. Feel free to open another PR to discuss possible implementations. |
Thanks for the headsup, i'll see if there's a way in which we can implement this without a breaking change |
|
thanks @wmwart for the excellent example , i'll give this a shot and see how it goes! |
@andrico1234 thanks for your work on this – I'd really appreciate this feature! |
* update window title on page change * remove use of UMD global * pass through custom empty component to Datagrid * don't update title if component is an element, update the doc title to be the same as page title * fix example * move functions and components into their own files * update readme * reset createedit readme * Update docs/CreateEdit.md Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com> * Update packages/ra-core/src/util/useDocumentTitle.ts Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com> * use export * syntax * remove unused exports * test * make changes * make example consistent * revert formatting changes Co-authored-by: HH: Andrico Karoulla <andrico@hhs-macbook-pro.home> Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com>
Closes: #5893
Updates the window title to match with the title of the current page. This solves an issue with the browser history simply displaying the same title for every single page.
Note: I created a new hook called useDocumentTitle this is a way to set the document title. I added it to the hooks folder in ra-core and exported it. To me it doesn't feel like it should live there. Practically it would only be used in this Title component, so I'm not sure if we should keep the function located in ra-core, but there aren't any hooks exported in ra-ui-materialui`.
Saying that, if someone chooses to pass through their own Title component, they should be able to manage their document.title however they see fit, which means they should also be able to import useDocumentTitle.
tl;dr, useDocumentTitle lives in ra-core but i'll be happy to place it in ra-ui-materialui if you see it fit