-
Notifications
You must be signed in to change notification settings - Fork 148
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
feat: Add task request details page #404
Conversation
Deploying with Cloudflare Pages
|
fa223c5
to
338204f
Compare
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.
Loved the utility functions for creating the custom elements 🙌🏻
taskRequests/details/script.js
Outdated
taskSkeleton.classList.add('hidden'); | ||
const data = await res.json(); | ||
|
||
const { taskData } = data; |
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.
The taskData should be initialised with an {} so that we can avoid undefined errors
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.
addressed, pls check
taskRequests/details/script.js
Outdated
}), | ||
); | ||
} catch (e) { | ||
console.log(e); |
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.
Please use console.error instead
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.
addressed
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.
Left a few comments
63a2ede
to
2eff3ef
Compare
Use console.error in catch statement Avoid unexpected error due to undefined value
2eff3ef
to
8d33633
Compare
PR for tests: #407 |
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.
Can we keep the path as /tasks itself where we can enlarge a task and request for it?
camel casing in a URL looks odd.
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.
Please write tests for these changes
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.
LGTM
Overview
This PR is a part of stacked PR pattern, The PR below it are:
Tests PR
Desktop view
status waiting
Show
status approved
Show
Show
Mobile View
status approved
Show
status waiting
Show
loading
Show