Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(ws): add namespace dropdown to UI #154

Merged
merged 3 commits into from
Jan 9, 2025
Merged

feat(ws): add namespace dropdown to UI #154

merged 3 commits into from
Jan 9, 2025

Conversation

YosiElias
Copy link
Member

image
image

paulovmr
paulovmr suggested changes Dec 5, 2024
View reviewed changes
Copy link

@paulovmr paulovmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @YosiElias , thanks for the PR! I added a few comments inline, and I would like to ask you a few things to align this PR with the Kubeflow contributing guide and to make the review easier:

  • I can see a few extra commits in this PR's branch, please consider adding just the one related to your changes, with your sign-off.
  • To follow the commit naming convention, please consider having the text "feat(ws): Notebooks 2.0 // Frontend // Namespace selector" for the commit message and PR label.

Also please consider adding some Cypress tests to automate the namespace changing verification. Thanks!

workspaces/frontend/src/app/DropDwon.tsx Outdated Show resolved Hide resolved
workspaces/frontend/src/app/DropDwon.tsx Outdated Show resolved Hide resolved
workspaces/frontend/src/app/hooks/useMount.tsx Outdated
@@ -0,0 +1,9 @@
import { useEffect } from "react"

const useMount = (callback:()=>void): void => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the useEffect could be used directly, without this hook.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a preparation for future useEffect calls without dependencies, meaning, it's much for "readable" and compact this way in our opinion (one line simple call wrapping a function, instead of whole useEffect syntax)
If you still think a direct useEffect call is better, we will refactor it back.

workspaces/frontend/src/app/DropDwon.tsx Outdated Show resolved Hide resolved
@thesuperzapper thesuperzapper changed the title Namespace UI feat(ws): add namespace dropdown to UI Dec 11, 2024
@YosiElias YosiElias force-pushed the namespace-ui branch 3 times, most recently from 72f6ab6 to fee7584 Compare December 17, 2024 14:54
Griffin-Sullivan
Griffin-Sullivan reviewed Dec 17, 2024
View reviewed changes
Copy link

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make the changes I suggested, you can then run the following in the frontend/ dir:

npm run test:fix

and that should fix your CI failure.

workspaces/frontend/src/app/context/NamespaceContextProvider.tsx Outdated Show resolved Hide resolved
workspaces/frontend/src/app/hooks/useMount.tsx Outdated Show resolved Hide resolved
@google-oss-prow google-oss-prow bot added size/XXL and removed size/L labels Dec 29, 2024
@YosiElias
Copy link
Member Author

YosiElias commented Dec 29, 2024 •
edited
Loading

Please note that, to unblock others, #167 needs to be merged before this PR (I'll rebase after #167 will be merged)

@YosiElias YosiElias force-pushed the namespace-ui branch 9 times, most recently from f830c37 to 3a985e3 Compare January 5, 2025 12:13
yelias added 2 commits January 7, 2025 08:59
feat(ws): Notebooks 2.0 // Frontend // Namespace selector
c1111b8 
Signed-off-by: yelias <yossi.elias@nokia.com>
add MenuSearch
ca76781 
Signed-off-by: yelias <yossi.elias@nokia.com>
@google-oss-prow google-oss-prow bot added size/L and removed size/XXL labels Jan 7, 2025
Griffin-Sullivan
Griffin-Sullivan approved these changes Jan 8, 2025
View reviewed changes
Copy link

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good nice work!

paulovmr
paulovmr suggested changes Jan 8, 2025
View reviewed changes
Copy link

@paulovmr paulovmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @YosiElias , just a few small comments, otherwise LGTM!

workspaces/frontend/src/__tests__/cypress/cypress/tests/e2e/NamespaceSelector.cy.ts Outdated Show resolved Hide resolved
workspaces/frontend/src/app/context/NamespaceContextProvider.tsx Outdated Show resolved Hide resolved
workspaces/frontend/src/app/context/NamespaceContextProvider.tsx Outdated Show resolved Hide resolved
feat(ws): Notebooks 2.0 // Frontend // Namespace selector
8552a96 
Signed-off-by: yelias <yossi.elias@nokia.com>
@YosiElias
Copy link
Member Author

Thank you @paulovmr and @Griffin-Sullivan for the comments!
Everything is fixed and ready to merge.

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, Griffin-Sullivan, paulovmr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ederign
Copy link
Member

ederign commented Jan 9, 2025

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 9, 2025
@ederign
Copy link
Member

ederign commented Jan 9, 2025

Thank you for the PR @YosiElias !

@google-oss-prow google-oss-prow bot merged commit 57fc8b1 into kubeflow:notebooks-v2 Jan 9, 2025
4 checks passed
@YosiElias YosiElias mentioned this pull request Jan 14, 2025
Closed
@thesuperzapper thesuperzapper mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asaadbalum asaadbalum asaadbalum left review comments

@jenny-s51 jenny-s51 jenny-s51 left review comments

@Griffin-Sullivan Griffin-Sullivan Griffin-Sullivan approved these changes

@paulovmr paulovmr paulovmr approved these changes

@ederign ederign ederign approved these changes

@kimwnasptd kimwnasptd Awaiting requested review from kimwnasptd

Assignees

@ederign ederign

Labels
approved lgtm size/L
Projects
Kubeflow Notebooks
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@YosiElias @ederign @paulovmr @jenny-s51 @Griffin-Sullivan @asaadbalum