-
Notifications
You must be signed in to change notification settings - Fork 504
Fe 150/api key management section #3147
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
base: epic/api-v2
Are you sure you want to change the base?
Conversation
enzo-bitfly
left a comment
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.
great work, and also great that you updated the README!
| authRouter.HandleFunc("/notifications-center/removeall", handlers.RemoveAllValidatorsAndUnsubscribe).Methods("POST") | ||
|
|
||
| authRouter.HandleFunc("/subscriptions/data", handlers.UserSubscriptionsData).Methods("GET") | ||
| authRouter.HandleFunc("/generateKey", handlers.GenerateAPIKey).Methods("POST") |
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.
issue: we should probably remove everything related to the old API key generation (handlers, templates, and this route - also some DB calls) - maybe @LuccaBitfly can help.
Can be done after this PR.
go.mod
Outdated
| go.opentelemetry.io/otel/trace v1.31.0 // indirect | ||
| go.uber.org/multierr v1.11.0 // indirect | ||
| go.uber.org/zap v1.27.0 // indirect | ||
| golang.org/x/tools v0.29.0 // indirect |
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.
question: is it expected that all these modules are deleted?
templates/user/settings.html
Outdated
| {{ end }} | ||
| {{ end }} | ||
|
|
||
| <div class="alert alert-info alert-dismissible fade show my-3 p-3 d-flex " role="alert"> |
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.
issue: alert is meant for dynamic changes, and it shouldn't contain interactive elements. You could use role="region" (or use a <section> with a heading) to convey to assistive technologies that this message is important.
| } | ||
| </style> | ||
| {{ end }} | ||
| {{ define "user/api-key-management/create" }} |
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.
question: why is it necessary to define the whole path here?
static/api-key-management/list.ts
Outdated
|
|
||
| export function initApiKeysTable(api: ApiKeysClient, opts: InitOpts) { | ||
|
|
||
| const dt = $(opts.tableSelector).DataTable({ |
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.
style: let's try to stick to complete variable names at least for frontend code.
static/api-key-management/modal.ts
Outdated
| onHidden?: () => void; | ||
| }; | ||
|
|
||
| export function useBSModal(modalEl : HTMLElement, options: BSModalOptions = {}) { |
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.
issue: BS is a third party library. The idea is that we wrap it in our custom reusable logic (this function) and what we expose to our application doesn't mention that. So useModal should be a good name for this.
4753366 to
e27c82e
Compare
98289f0 to
10fc526
Compare
See: FE-153
0c83f54 to
0217e48
Compare
e359f97 to
037e84a
Compare
See: FE-150 See: FE-156 See: FE-165 See: FE-166 See: FE-167
037e84a to
1c26cee
Compare
d038359 to
770d106
Compare
No description provided.