Conversation
czezi
force-pushed
the
integrations/github
branch
from
deb4bb0 to
0c88ada
Compare
|
|
||
| export const deleteGithubIntegration = async () => { | ||
| const response = await api.delete(`/integrations/github`); | ||
| return response.data; |
There was a problem hiding this comment.
what will happen if the API call fails, in that case we won't have data plus there is no error handling here. Better to move the retrieval and error handling outside the Dashboard Service layer
There was a problem hiding this comment.
You operate on the retrieved data or catch it and decide what to do later. You separate multiple layers and reuse based on use case. Never mix api calls and ui actions, it's an anti pattern.
gui/src/app/projects/page.tsx
Outdated
| {project.project_repository && ( | ||
| <div className="flex items-center"> | ||
| <div className="-mt-1 mr-2 h-3.5 w-3.5"> | ||
| <svg viewBox="0 0 24 24" className="fill-current"> |
There was a problem hiding this comment.
sir please add the image in the public dir, add that image to imagePath and then use it from there
| > | ||
| <div className="flex items-center"> | ||
| <div className="mr-4 h-10 w-10"> | ||
| <svg viewBox="0 0 24 24" className="fill-current"> |
There was a problem hiding this comment.
same here move the images to public dir
| {isExternalGitIntegration === undefined ? ( | ||
| <></> | ||
| ) : isExternalGitIntegration === false ? ( | ||
| <button |
There was a problem hiding this comment.
please use from next ui instead of standard button tag
There was a problem hiding this comment.
Can't use standard button, link kind of button isn't supported by nextui.
| .integrate_github_container { | ||
| background-color: #262628; | ||
| border-radius: 0.5rem; | ||
| border-left: #a8a8a8 0.3rem solid; |
There was a problem hiding this comment.
the similar hexcode is already define so please use it
There was a problem hiding this comment.
Using colors with transparency is a very bad idea, they look different on different backgrounds, please fix it when you get time.
| project_description: string; | ||
| project_hash_id: string; | ||
| project_url: string; | ||
| project_repository: string | undefined; |
There was a problem hiding this comment.
in ts, both null and undefined are treated as two different distinct types and mostly we deal with null than undefined so add that as well
There was a problem hiding this comment.
It's against typescript standard, https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined
| Repository | ||
| </label> | ||
| <div className="flex items-center space-x-4"> | ||
| <label className="flex items-center"> |
There was a problem hiding this comment.
this radio component is way to length, make it into a component and use it here. create a dir for it and module css for the same component
There was a problem hiding this comment.
Doesn't make sense, this radio button has a defined structure no evidence it'll be reusable later.
| clearInterval(interval); | ||
| } | ||
| }, | ||
| 1000, |
There was a problem hiding this comment.
setInterval function normally takes only two primary arguments, the callback and the delay, so Ig the second 1000 is redundant.
czezi
force-pushed
the
integrations/github
branch
from
5aa1c6e to
361293e
Compare
2 participants
Description
Related Issues
Solution and Design
Test Plan
Type of change
Checklist