fix: Tab doesn't represent the page you are on for non-data asset pages #9468
Conversation
| @@ -15,11 +16,13 @@ export default function AppProviders({ children }: Props) { | |||
| <AppConfigProvider> | |||
| <UserContextProvider> | |||
| <EntityRegistryProvider> | |||
| <BrowserTitleProvider> | |||
There was a problem hiding this comment.
Code styling: This component should be indented to match the others please!
There was a problem hiding this comment.
I will update it cause due to conflict
| @@ -140,6 +164,9 @@ export const SearchablePage = ({ onSearch, onAutoComplete, children }: Props) => | |||
| authenticatedUserPictureLink={user?.editableProperties?.pictureLink} | |||
| entityRegistry={entityRegistry} | |||
| /> | |||
| <Helmet> | |||
There was a problem hiding this comment.
Do we need helmet here?
Ideally the Helmet is defined higher ups in the component hierarchy so that we can change the title for ANYWHERE inside the application
There was a problem hiding this comment.
Helmet is used in two places APP.tsx and EntityHead. In EntityHead other routes are not wrap and In App.tsx all routes are wrap but dynamicThemeConfig is added in json which is not in context that's the reason I have create new component
| const { title, updateTitle } = useBrowserTitle(); | ||
|
|
||
| useEffect(() => { | ||
| // Update the title only if it's not already set and there is a valid pathname |
There was a problem hiding this comment.
@aseembansal-gogo Use case check:
What this code does is it splits the path of the URL itself to determine the title of the page.
For example this will do:
Dataset | Urn:li:sample:etc
For entity pages.
I don't think this is what we want. Please chime in with requirements clarifications.
There was a problem hiding this comment.
/settings/identities/users -> Settings | Identities | Users
Checklist