Skip to content

Move page route to /@shortname/<pageEntityUuid>#1650

Merged
benwerner01 merged 18 commits intomainfrom
bw/refactor-page-route
Dec 15, 2022
Merged

Move page route to /@shortname/<pageEntityUuid>#1650
benwerner01 merged 18 commits intomainfrom
bw/refactor-page-route

Conversation

@benwerner01
Copy link
Copy Markdown
Contributor

@benwerner01 benwerner01 commented Dec 12, 2022

🌟 What is the purpose of this PR?

This PR moves pages from /<accountId>/<pageEntityUuid> to /@shortname/<pageEntityUuid>.

🔗 Related links

🔍 What does this change?

  • adds a getServerSideProps function for pages to get the pageEntityId and the pageWorkspace on first render
  • renames the RouteAccountInfo react context to RouteWorkspaceInfo

📜 Does this require a change to the docs?

Not that I'm aware of.

⚠️ Known issues

  • The pageEntityId is re-calculated in the sidebar even though it has already been calculated in getServerSideProps

🛡 What tests cover this?

None. This PR was manually tested.

❓ How to test this?

  1. Run the HASH App locally, and log in
  2. Navigate to a page, and observe the shortname in the URL. Ensure the page is behaving as expected.

📹 Demo

@github-actions github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) frontend labels Dec 12, 2022
@benwerner01 benwerner01 changed the base branch from main to bw/homepage-ssr December 12, 2022 17:16
Base automatically changed from bw/homepage-ssr to main December 12, 2022 17:46
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch from 0236249 to 7963159 Compare December 12, 2022 18:17
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch 2 times, most recently from 47e556e to 55db0e7 Compare December 12, 2022 18:26
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch from 55db0e7 to ec0a84f Compare December 12, 2022 18:37
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch from ec0a84f to 5b262e5 Compare December 12, 2022 18:49
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch from 5b262e5 to 3eb5746 Compare December 12, 2022 18:54
@benwerner01 benwerner01 force-pushed the bw/refactor-page-route branch from 3eb5746 to b80f17a Compare December 12, 2022 19:00
@benwerner01 benwerner01 marked this pull request as ready for review December 12, 2022 19:00
Comment on lines +24 to +26
/** @todo: re-use the `pageEntityId` provided by `getServerSideProps` */
const { routePageEntityId } =
useRoutePageInfo({ allowUndefined: true }) ?? {};
Copy link
Copy Markdown
Contributor Author

@benwerner01 benwerner01 Dec 12, 2022

Choose a reason for hiding this comment

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

I wasn't sure what the best way of avoiding this would be, as the sidebar is not rendered as a child of the [page-slug].page.tsx page. Potentially by having some shared context, that the [page-slug].page.tsx page sets on first-render? Curious to get thoughts (cc @nathggns @Alfred-Mountfield @yusufkinatas @luisbettencourt)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Potentially Next 13 layouts could help with this also, but I'm not too sure...

Copy link
Copy Markdown
Contributor Author

@benwerner01 benwerner01 Dec 15, 2022

Choose a reason for hiding this comment

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

Discussed this internally, and came to the realisation that the pageEntityUuid could be used instead. Have addressed in 48aab54

@benwerner01 benwerner01 changed the title Move page route from /<accountId>/<pageEntityUuid> to /@shortname/<pageEntityUuid> Move page route to /@shortname/<pageEntityUuid> Dec 13, 2022
@thehabbos007 thehabbos007 mentioned this pull request Dec 13, 2022
2 tasks
@CiaranMn CiaranMn requested a review from kachkaev December 14, 2022 10:13
@benwerner01 benwerner01 requested a review from kachkaev December 15, 2022 11:38
Copy link
Copy Markdown
Contributor

@kachkaev kachkaev left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments @benwerner01! This part of the diff looks great, so happy for someone in the backend team to resolve remaining discussions and approve this PR. We’ll also need to check why the Playwright tests are failing. The logs suggest that it’s something related to routing.

thehabbos007
thehabbos007 previously approved these changes Dec 15, 2022
Copy link
Copy Markdown
Contributor

@thehabbos007 thehabbos007 left a comment

Choose a reason for hiding this comment

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

Great stuff!

@benwerner01 benwerner01 merged commit 51c3c38 into main Dec 15, 2022
@benwerner01 benwerner01 deleted the bw/refactor-page-route branch December 15, 2022 16:17
@vilkinsons vilkinsons added type/eng > frontend Owned by the @frontend team area/tests > playwright New or updated Playwright tests and removed area: frontend labels Jul 19, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.24%. Comparing base (f4faf5a) to head (c013779).
Report is 4707 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1650   +/-   ##
=======================================
  Coverage   58.24%   58.24%           
=======================================
  Files         222      222           
  Lines       15794    15794           
  Branches      378      378           
=======================================
  Hits         9199     9199           
  Misses       6590     6590           
  Partials        5        5           
Flag Coverage Δ
backend-integration-tests 2.70% <ø> (ø)
unit-tests 1.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/tests > playwright New or updated Playwright tests type/eng > frontend Owned by the @frontend team

Development

Successfully merging this pull request may close these issues.

4 participants