Skip to content

Improve Submissions integration UI#77

Merged
3mcd merged 5 commits into
mainfrom
em/submissions-ui
Sep 9, 2023
Merged

Improve Submissions integration UI#77
3mcd merged 5 commits into
mainfrom
em/submissions-ui

Conversation

@3mcd
Copy link
Copy Markdown
Collaborator

@3mcd 3mcd commented Sep 8, 2023

This PR:

  • makes the unjournal seed script aware of local dev environments (the submissions integration will now link to localhost:3002 properly)
  • makes integrations load instance configuration (if any) and user info from the root layout
  • adds a bunch of shadcn components and uses them in the submissions integration
  • adds a new @pubpub/sdk/react entrypoint that exports a few useful boilerplate components for integrations

Most changes are cosmetic, but I would like opinions on fetching data from the root layout, because I'm not sure if that's a good idea or not.

Issue(s) Resolved

N/A

Test Plan

  1. Go through the test plan in Early Submissions integration #69 on your local machine (the review app works but you have to mess with the URLs). Everything should work the same as in Early Submissions integration #69, but it looks better now!

Screenshots (if applicable)

image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed some typical key warnings.

Copy link
Copy Markdown
Collaborator Author

@3mcd 3mcd Sep 8, 2023

Choose a reason for hiding this comment

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

I was able to get tailwind working with dependencies by resolving absolute paths of the packages.

Comment thread integrations/submissions/middleware.ts Outdated
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This might not be the best approach, but after a few hours of research, this seems like the only/best way to reliably get search parameters in layout components.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the reason Next makes doing this clunky is because they're trying to enforce that if your layout needs to know search params, there's something funky in the route architecture. Since layouts apply to all children (e.g. /, /blog, /blog/1) - they're suggesting that it would be funky to have that top level layout consistently require search params all the way down (as opposed to say, using those params at the appropriate /page.tsx and doing any necessary fetch there).

I understand why you've implemented the approach as you did here, but I can see Next's point too. Perhaps we could load something like /authenticate/page.tsx which reads the search params, and then sets a cookie/localStorage before redirecting to / so that intra-app links aren't required to pass searchParams around forever.

I've fought with Next about this before, and in the end almost always decided that Next was right — and if I thought I needed searchParams in layout.tsx as opposed to page.tsx I was deviating from the intended use of layout and page files.

That said, if it's working - it's working :) And I know there's a deadline, so pure code alignment with Next's opinions doesn't need to be first priority.

Copy link
Copy Markdown
Collaborator Author

@3mcd 3mcd Sep 8, 2023

Choose a reason for hiding this comment

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

That makes sense. Thanks for the insight. We've talked about using a cookie anyways, so I'll take a stab at the page approach!

Copy link
Copy Markdown
Member

@isTravis isTravis left a comment

Choose a reason for hiding this comment

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

I think it'd be fine to merge this with the middleware approach. That could always be refactored at a later date. It seems the higher priority is just getting the functionality pushed forward.

If you can see the refactor you'd want to make now though, of course go for it, but I don't think any of the code structure as is should be a blocker for merging.

@3mcd 3mcd force-pushed the em/submissions-ui branch from e734bf2 to 78031ad Compare September 9, 2023 00:34
@isTravis isTravis temporarily deployed to em/submissions-ui - core PR #77 September 9, 2023 00:34 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/submissions-ui - integration-evaluations PR #77 September 9, 2023 00:34 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/submissions-ui - integration-submissions PR #77 September 9, 2023 00:34 — with Render Destroyed
@3mcd 3mcd merged commit 624ffd6 into main Sep 9, 2023
@3mcd 3mcd deleted the em/submissions-ui branch September 9, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants