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.

Sign up for GitHub

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

Jump to bottom

feat: Improved Page and History names #4908

Merged
merged 8 commits into from
Apr 15, 2024
Merged

feat: Improved Page and History names #4908

merged 8 commits into from
Apr 15, 2024

Conversation

zvolcsey
Copy link
Contributor

@zvolcsey zvolcsey commented Apr 10, 2024 •
edited

Hi! I improved page and history names.
Closes #4684

page-title.mp4
settings.mp4

  1. Page Title of Workspace objects
    I added the PageTitle component to the ViewPickerDropdown component. If the user changes the view, then the page title is updated.
    All Comapnies - Companies
    Best ones - Companies
    By Stage - Oppurtunites

  2. Object Singular
    I added the PageTitle component to the RecordShowPage component.

  3. Settings (and Tasks page)
    I added the paths from SettingsPath to the title-utils.ts.
    https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/modules/types/SettingsPath.ts
    For example:
    title-utils
    Except:
    For these paths, I added the PageTitle component to the corresponding Route component.

  • ObjectDetail -> Object Name - Settings
  • ObjectEdit -> Edit Object Name - Settings
  • ObjectNewField (Step1 and Step2) -> Step number New Object Name Field - Settings
  • DevAPIKeyDetail -> API Key Name API Key - Settings
  • IntegrationDB -> DBName Database - Settings, Airtable Database - Settings
  • IntegrationDBConnection -> Connection Name Connection - Settings
  • IntegrationNewDBConnection -> New Airtable Connection - Settings
  • WebhookDetail -> Webhook Detail - Settings

@martmull martmull self-assigned this Apr 11, 2024
@charlesBochet charlesBochet assigned ijreilly and unassigned martmull Apr 11, 2024
ijreilly
ijreilly reviewed Apr 12, 2024
View reviewed changes
Copy link
Contributor

@ijreilly ijreilly left a comment

Choose a reason for hiding this comment

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

Thank you for contributing @zvolcsey :) !

packages/twenty-front/src/modules/views/view-picker/components/ViewPickerDropdown.tsx Outdated
@@ -86,6 +91,11 @@ export const ViewPickerDropdown = () => {
) : (
<IconList size={theme.icon.size.md} />
)}
<PageTitle
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic could be moved up to ViewBar which seems to be the parent component among all view components

packages/twenty-front/src/modules/views/view-picker/components/ViewPickerDropdown.tsx Outdated
<PageTitle
title={`${
currentViewWithCombinedFiltersAndSorts?.name ?? 'All'
} - ${capitalize(objectNamePlural)}`}
Copy link
Contributor

@ijreilly ijreilly Apr 12, 2024 •
edited

Choose a reason for hiding this comment

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

'All' - ${capitalize(objectNamePlural)} does not work so well here if objectNamePlural is equal to '' which is possible in theory. But with the logic moved with ViewBar you will not have to worry about this since objectPluralName is defined for sure there

packages/twenty-front/src/pages/not-found/NotFound.tsx Outdated
@@ -36,6 +37,7 @@ export const NotFound = () => {

return (
<>
<PageTitle title="Page Not Found" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% sure it is what we would want here. @Bonapara ?

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 for the 404?

Page Not Found | Twenty maybe?

packages/twenty-front/src/modules/views/view-picker/components/ViewPickerDropdown.tsx Outdated
@@ -86,6 +91,11 @@ export const ViewPickerDropdown = () => {
) : (
<IconList size={theme.icon.size.md} />
)}
<PageTitle
title={`${
currentViewWithCombinedFiltersAndSorts?.name ?? 'All'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not mandatory but I think we could also add a PageTitle within RecordIndexPage to handle "All Companies - Companies" (or maybe just "Companies"). In practice that will always be overwritten by the title logic in ViewBar because in today's setup RecordIndexPage always has ViewBar as a child component but it could easily stop being the case (what if we want to have a read-only view that would not feature the ViewBar to edit it?).

Ideally all the records titling logic would be at the same place in RecordIndexPage using useGetCurrentView to fetch the view information but the hook is not usable as is in RecordIndexPage at the moment so let's not make things complex.

packages/twenty-front/src/utils/title-utils.ts Outdated
@@ -19,13 +19,39 @@ export const getPageTitleFromPath = (pathname: string): string => {
case AppPath.OpportunitiesPage:
return 'Opportunities';
case `${AppBasePath.Settings}/${SettingsPath.ProfilePage}`:
return 'Profile';
return 'Profile - Settings';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only want to have the values Appearance - Settings, Profile - Settings (for all children pages of Profile, except Appearance that is in the product not a child of Profile but that seems to be by the path: settings/profile/appearance), Accounts - Settings (for all children pages of Accounts: Emails, Calendars..).
We don't need more granularity that could be hard to maintain as we add more pages/components

packages/twenty-front/src/pages/settings/data-model/SettingsObjectDetail.tsx Outdated
@@ -90,6 +91,9 @@ export const SettingsObjectDetail = () => {

return (
<SubMenuTopBarContainer Icon={IconSettings} title="Settings">
<PageTitle
Copy link
Contributor

Choose a reason for hiding this comment

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

As said in my above comment in title-utils I don't think we want that granularity in those components

@zvolcsey
Copy link
Contributor Author

zvolcsey commented Apr 14, 2024 •
edited

@ijreilly Hi! Thank you for the review! I fixed the things mentioned above.

  • Move PageTitle to the ViewBar component and to the RecordIndexPage component (here the name is the capitalized objectNamePlural e.g.: Companies)
  • Remove the unnecessary fallback value (All) from ViewBar component.
  • Change the name of the 404 page to Page Not Found | Twenty.
  • Remove unnecessary PageTitle components from components.
  • Remove unnecessary titles from title-utils.ts and title-utils.test.ts.

@ijreilly
Copy link
Contributor

@zvolcsey thank you for your great work and for being so responsive.
I added a few minor changes, and we should be ready for merge. thanks!!

ijreilly
ijreilly approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@ijreilly ijreilly left a comment

Choose a reason for hiding this comment

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

thx @zvolcsey !

@ijreilly ijreilly merged commit 5477665 into twentyhq:main Apr 15, 2024
4 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bonapara Bonapara Bonapara left review comments

@ijreilly ijreilly ijreilly approved these changes

Assignees

@ijreilly ijreilly

Labels
PR: awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Page Name & Browser History
5 participants
@zvolcsey @ijreilly @Bonapara @charlesBochet @martmull