-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix: Adds title of tool to feedback commit #226
base: main
Are you sure you want to change the base?
Fix: Adds title of tool to feedback commit #226
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @letzfets! Thank you for taking a look at this!
I've added some comments with feedback, and look forward to work together to get this ready and merged! 😄
const body = `${disclaimer} | ||
${userContent} | ||
--- | ||
- URL: ${url} | ||
- Git commit: \`${GIT_COMMIT}\` | ||
|
||
--- | ||
|
||
${userContent}` | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works just as expected - nice work!
I'm changing the formatting slightly to clearly separate the disclaimer from the userContent
.
const body = `${disclaimer} | |
${userContent} | |
--- | |
- URL: ${url} | |
- Git commit: \`${GIT_COMMIT}\` | |
--- | |
${userContent}` | |
` | |
const body = `${disclaimer} | |
--- | |
${userContent} | |
--- | |
- URL: ${url} | |
- Git commit: \`${GIT_COMMIT}\`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes it look nicer! 👍
@@ -51,6 +53,7 @@ export const actions: Actions = { | |||
if (!Boolean(data.liked || data.improve)) return { success: false } | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can load the specific tool referenced for this request, and then use the name when submitting the issue.
To accomplish this, we need to
- update the parameters for the
default
action to includeparams: { link }
.
- default: async ({ request, url }) => {
+ default: async ({ request, url, params: { link } }) => {
- load the tool, and verify that it exists before continuing.
const tool = getToolByLink(link, content) | |
if (!tool) return { success: false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will load the tool twice for each page from the store: once in the load()
function and then again for the feedback submit form via actions
.
I have defined tool globally, but the way I did it, the load()
function is required to run before actions
- not a nice dependency either, because as soon as load()
changes, the actions
might be broken. Is there a way to only load the tool
once for each page? Or is it less of importance how many reads we do to the database? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for having a look at this.
It starts getting better and better. 😉
What do you think about loading the tool
twice per page?
const body = `${disclaimer} | ||
${userContent} | ||
--- | ||
- URL: ${url} | ||
- Git commit: \`${GIT_COMMIT}\` | ||
|
||
--- | ||
|
||
${userContent}` | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes it look nicer! 👍
@@ -51,6 +53,7 @@ export const actions: Actions = { | |||
if (!Boolean(data.liked || data.improve)) return { success: false } | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will load the tool twice for each page from the store: once in the load()
function and then again for the feedback submit form via actions
.
I have defined tool globally, but the way I did it, the load()
function is required to run before actions
- not a nice dependency either, because as soon as load()
changes, the actions
might be broken. Is there a way to only load the tool
once for each page? Or is it less of importance how many reads we do to the database? 🤔
Fixes issue #220