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.

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

Router: <Set wrap={}> #1898

Merged
merged 5 commits into from
Mar 19, 2021
Merged

Router: <Set wrap={}> #1898

merged 5 commits into from
Mar 19, 2021

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Mar 3, 2021

This PR adds support for <Set wrap={}>, that allows users to create sets of routes and wrap them with layouts and other components.

This is the implementation that came out of the discussion over on our forums: https://community.redwoodjs.com/t/redwood-router-with-layouts-context-providers-etc/1342

@github-actions
Copy link

github-actions bot commented Mar 3, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/create-redwood-app-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-api-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-api-server-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-auth-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-cli-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-core-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-dev-server-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-eslint-config-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-eslint-plugin-redwood-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-forms-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-internal-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-prerender-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-router-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-structure-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-testing-0.27.1-b4c7469.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1898/redwoodjs-web-0.27.1-b4c7469.tgz

Install this PR by running yarn rw upgrade --pr 1898:0.27.1-b4c7469

@Tobbe Tobbe force-pushed the tobbe-router-set branch 3 times, most recently from 9522b0b to d1873ab Compare March 3, 2021 13:20
@Tobbe Tobbe force-pushed the tobbe-router-set branch from d1873ab to 45ba8a0 Compare March 3, 2021 14:27
@dthyresson
Copy link
Contributor

Given there was some last minute discussion over naming in Prerender, is there consensus among using the terms "Set" and "wrap"?

It feels like one is adding some UI information to the routing by wrapping the routing it the way the page is rendendered.

But, now that I type that I realize that declaring and Page and the prerender option is doing just exactly that.

Some other options:

wrap -> render
Set -> Routes (plural)

others?

@Tobbe Tobbe force-pushed the tobbe-router-set branch 3 times, most recently from fe9ba3a to 82c5db5 Compare March 4, 2021 08:28
@Tobbe
Copy link
Member Author

Tobbe commented Mar 4, 2021

Given there was some last minute discussion over naming in Prerender, is there consensus among using the terms "Set" and "wrap"?

We iterated on a few different names, and this was @mojombo's latest suggestion. I liked it, and no one complained or came with any other suggestions after that, so that's what I went with.

@Tobbe Tobbe force-pushed the tobbe-router-set branch 3 times, most recently from 74d197b to baa6d7c Compare March 10, 2021 12:54
@Tobbe Tobbe force-pushed the tobbe-router-set branch 9 times, most recently from 9c4699e to ed3dc2d Compare March 14, 2021 21:32
@Tobbe Tobbe force-pushed the tobbe-router-set branch 3 times, most recently from 2ab08b0 to 15381e4 Compare March 17, 2021 21:14
@thedavidprice
Copy link
Contributor

Can anyone remind me where the final RFC design + scope for this ended up? Was trying to find an issue for reference. @Tobbe Might be nice to include the original conversation in your OP.

Thanks for cranking on this, Tobbe!

@@ -7,19 +7,19 @@ const createHistory = () => {
listen: (listener: Listener) => {
const listenerId = 'RW_HISTORY_LISTENER_ID_' + Date.now()
listeners[listenerId] = listener
window?.addEventListener('popstate', listener)
global.addEventListener('popstate', listener)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok for these to not use the optional chaining operator ?.

Copy link
Member Author

Choose a reason for hiding this comment

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

When is global undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant for the execution of the functions; global.addEventListener?.()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, should be fine. We're not triggering any navigations when prerendering, are we? So, even using window would have been fine. But to be consistent I'll use global

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

So close now!

@Tobbe Tobbe force-pushed the tobbe-router-set branch 2 times, most recently from e9d633d to b4c7469 Compare March 19, 2021 08:19
@Tobbe Tobbe merged commit 438efbc into redwoodjs:main Mar 19, 2021
@Tobbe Tobbe deleted the tobbe-router-set branch March 19, 2021 08:31
dac09 added a commit to dac09/redwood that referenced this pull request Mar 22, 2021
…r-rwt-link

* 'main' of github.com:redwoodjs/redwood: (26 commits)
  Router: Fix auth (redwoodjs#2038)
  Steps towards a11y for Redwood Router (redwoodjs#1817)
  Router: <Set wrap={}> (redwoodjs#1898)
  Pass event and context into getCurrentUser (redwoodjs#1908)
  Implement Redwood API side Logger (redwoodjs#1937)
  Fixed path on windows to allow for pages under subdirectories (redwoodjs#2022)
  Add experimental ESBuild for api side (redwoodjs#1948)
  Upgrade to Prisma 2.19.0 (Prisma Migrate GA) (redwoodjs#2021)
  Fix lint breaking when deleting a side (redwoodjs#2017)
  Refactor: Converted Prisma.ts to js (redwoodjs#1958)
  Fix issue with verify email redirect using Auth0 (redwoodjs#1990)
  add GitHub Action CodeQL Analysis (redwoodjs#1951)
  fix: correct var name for grabbing user config (redwoodjs#2002)
  Create functions to fs calls (redwoodjs#2007)
  Return signup Output (redwoodjs#1992)
  Add makeExecSchema options (redwoodjs#1964)
  upgrade gotrue-js to 0.9.29 (redwoodjs#2011)
  Azure Active Directory Auth: Adding try-catch block on callback to capture empty key (redwoodjs#2010)
  withCellHOC: Fix TS error (redwoodjs#1967)
  Update error message in tasks/publish-local to point to tasks/run-local-npm when Verdaccio isn't running (redwoodjs#2004)
  ...
@thedavidprice thedavidprice added this to the next release milestone Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants