Merged
Conversation
🦋 Changeset detectedLatest commit: ac04830 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
9 tasks
For now, we'll only support existing projects. The button that links to the manual installation was just a blank rectangle – added some text to it.
Made some things a bit clearer - file paths - exporting the job rather than using a function wrapper - how to run the app and CLI dev command
matt-aitken
approved these changes
Sep 11, 2023
Member
There was a problem hiding this comment.
I made a few changes:
- The blank project page in the app was broken – there was a button with no text so it was just a rectangle. I also improved the text a bit.
- I made some of the docs a bit more specific
- I dealt with the code stripping a bit differently from how you suggested, by exporting instead. Neither are ideal to be honest but I think it's the best we can do because of how Remix works with side effects.
- Some tsconfig paths were missing
- "next" was a dependency of the remix package
- A specific version of Remix was in the main dependencies of the package. This would have caused issues with users who were on different versions. I moved it do be a devDependency only.
Shouldn't rely on a specific version in main dependencies. In this case we can just have devDependency for @remix/node
Member
|
@Chigala I made a few changes, detailed in the comment above. It would be great if you could look at your other adaptor PRs and make the same changes where they are relevant |
Contributor
Author
|
@matt-aitken Thanks for the changes you made! I'll update the other adaptor PR I sent to follow suit where relevant. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task 1 - Remix Adapter
remix.examples/remix-examplefolder.Task 2 - Add a README.md file
README.mdfile inside thepackages/remixfolder.Task 3 - Write the documentation
manual-setup-remix.mdxfile in/docs/_snippets/.client-adaptors.mdxpage with the remix framework.Task 4 - Update the webapp onboarding
route.tsxpage located in_app.orgs.$organizationSlug.projects.$projectParam.setup.remix, replaced the<FrameworkComingSoon/>component with new onboarding steps.FrameworkSelector.tsxfile, add "supported" to the framework you’ve worked on.I didn't include trigger init CLI support for the framework because I don't know what the Trigger team has in mind concerning how to handle initialization for the other separate frameworks. If this is needed I don't mind implementing it. I'll also love your feeback on this!
/claim #445
/closes #445