-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add advisory list page #20
Conversation
@helio-frota you seems to be passionate about this new repository. I'm not sure what would be your preference in regards of which part of the app you want to focus. Would you like to be more involved in the backend side or frontend side? What are your preferences? I know you can contribute in both, but at least frontend or backend would have If you have a preference for the UI side, can I ask you to review this PR? We can start small and start just reviewing the code based on But if your preference is backend, no worries, just forget what I wrote xD. Also I don't want to give you any work/task that you wound't take it voluntarily. Only your manager can assign you work :) |
I adjusted perms so that @helio-frota can be a reviewer/merger. |
yes 👍
rust mainly 👍
backend 👍
short -- rust long -- I'm thinking on re-writing my java background/history to rust
I never was a front-end dev... primefaces, richfaces and taglibs saved me in the past :D , and on the previous team I did other things. Probably 80%(BE)-20%(FE) (Pareto similar thing) is good for me on this repository and yeah I would like things going smooth and successful (and easy).
sure I'll review 👍 |
}, | ||
}; | ||
|
||
export const severityFromNumber = (score: number): Severity => { |
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.
Would it be useful to get words converted from numbers from the backend? Would future localizations be easier/harder? Should l10n stuff be only in the front-end, leaving the backend simpler?
I don't know nor do I have an opinion, just questions.
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.
Would it be useful to get words converted from numbers from the backend?
Definitely it would be easier. The current Trustification returns a number. But you're right, this is thing to enhance and leave it to the backend to interpret/convert scores (numbers).
So I'll leave it to the backend .
It seems I will need to redefine my DTOs (mock DTOs, not linked to the backend yet). https://github.com/carlosthe19916/trustify/blob/advisory-list/frontend/client/src/app/api/models.ts#L73
Would future localizations be easier/harder
localyzation should not be affected. It won't make it easier nor harder, just not affect.
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.
mock DTOs
+1
this is working 👍 I'm getting some weirdo errors and warnings but I know this is very early stages I'll take a look on the code 👍 |
That's not an |
@@ -15,6 +15,7 @@ export interface ConfirmDialogProps { | |||
| "danger" | |||
| "warning" | |||
| "info" | |||
/* eslint-disable @typescript-eslint/no-explicit-any */ |
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.
I saw others eslint inline here and there and again I think those things will be solved in the future so no problem for me.
useMemo | ||
/> | ||
</CardBody> | ||
</Card> */} |
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 can be removed ^ ?
// commented lines
isLineNumbersVisible | ||
isReadOnly | ||
isMinimapVisible | ||
// isLanguageLabelVisible |
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 be removed ?
// commented line
@@ -20,7 +20,6 @@ import { | |||
const __dirname = fileURLToPath(new URL(".", import.meta.url)); | |||
const pathToClientDist = path.join(__dirname, "../../client/dist"); | |||
|
|||
const brandType = TRUSTIFICATION_ENV.PROFILE; | |||
const port = parseInt(TRUSTIFICATION_ENV.PORT, 10) || 8080; | |||
|
|||
const app = express(); |
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 migrate this index.js to index.ts later 👍
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 the majority of the things are really related to UI components.
We know those things are better "reviewed" via e2e tests.
Other than that I added some nit comments 👍
thanks 👍 |
Changing the status to Draft as I don't know how to proceed. Should I just merge things?