-
Notifications
You must be signed in to change notification settings - Fork 30
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(ws): Notebooks 2.0 // Frontend // Workspaces details // Activity tab #171 #199
base: notebooks-v2
Are you sure you want to change the base?
Conversation
asaadbalum
commented
Feb 6, 2025
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.
Hi @asaadbalum , thanks for the PR! LGTM, I've left two inline comments about the naming. Also, there are some formatting issues, that can be fixed by npm run test:fix
.
Apart from that, the DCO check is failing on this PR.
workspaces/frontend/src/app/pages/Workspaces/Details/WorkspaceActivity.tsx
Outdated
Show resolved
Hide resolved
workspaces/frontend/src/app/pages/Workspaces/Details/WorkspaceActivity.tsx
Outdated
Show resolved
Hide resolved
13a4550
to
263118e
Compare
/ok-to-test |
/lgtm |
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.
Hi @asaadbalum, there is one important thing to change: the style of the overview tab (WorkspaceDetailsOverview.tsx
) was changed, so this PR should match it:
… tab kubeflow#171 Signed-off-by: Asaad Balum <[email protected]>
263118e
to
9ddd4ec
Compare
New changes are detected. LGTM label has been removed. |
Modified |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: paulovmr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hi @asaadbalum the code looks ok, but can you please introduce some tests for it? We need to improve our coverage.