Skip to content

feat(ui): remove notebook's unused buttons #7008

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
Merged
merged 5 commits into from
Apr 15, 2025
Merged

feat(ui): remove notebook's unused buttons #7008

merged 5 commits into from
Apr 15, 2025

Conversation

NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Feb 14, 2025 •
edited by bednar
Loading

Closes #6931

Remove two unused buttons in the Notebook screen for OSS:

Before:
360171779-018c7fb3-7698-43b7-aca1-7ed4942b36de
After:
Screenshot 2025-02-14 at 10 13 26

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Feb 14, 2025
@NguyenHoangSon96 NguyenHoangSon96 requested a review from a team as a code owner February 14, 2025 03:30
@NguyenHoangSon96 NguyenHoangSon96 linked an issue Feb 14, 2025 that may be closed by this pull request
Remove buttons for notebook versions in oss #6931
Closed
@NguyenHoangSon96 NguyenHoangSon96 changed the title Feat/remove notebook's unused buttons feat(ui): remove notebook's unused buttons Feb 14, 2025
bednar
bednar requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@bednar bednar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The source branch should be master; the OSS-2.7 branch is used for releases.

Removing the button should depend on the CLOUD property, because the source is shared between CLOUD and OSS. You can use something like:

ui/src/App.tsx

Line 146 in 5cfee32

{CLOUD ? (

@NguyenHoangSon96 NguyenHoangSon96 changed the base branch from OSS-2.7 to master February 19, 2025 01:19
@NguyenHoangSon96 NguyenHoangSon96 requested a review from a team as a code owner February 19, 2025 01:19
@NguyenHoangSon96
Copy link
Contributor Author

The source branch should be master; the OSS-2.7 branch is used for releases.

Removing the button should depend on the CLOUD property, because the source is shared between CLOUD and OSS. You can use something like:

ui/src/App.tsx

Line 146 in 5cfee32

{CLOUD ? (

Hi @bednar
I make some changes based on your request
Please check again

@philjb philjb requested a review from wdoconnell March 4, 2025 21:43
wdoconnell
wdoconnell reviewed Mar 5, 2025
View reviewed changes
Copy link
Contributor

@wdoconnell wdoconnell left a comment

Choose a reason for hiding this comment

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

Looks generally good to me. Thank you for doing this. I have one correction on an import and one suggestion about how to simplify the menuItems array.

@NguyenHoangSon96
Copy link
Contributor Author

Hi @wdoconnell. Thank for the review
I fixed all things you mentioned
Please check again

bednar

This comment was marked as outdated.

Sign in to view

bednar
bednar approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

From my perspective, the PR looks good and is ready for review by a UI maintainer.

@blegesse-w
Copy link
Member

From my perspective, the PR looks good and is ready for review by a UI maintainer.

everything looks good to me

@NguyenHoangSon96 NguyenHoangSon96 force-pushed the feat/remove-unused-buttons branch from 0262de8 to 4af9523 Compare April 15, 2025 07:29
@NguyenHoangSon96 NguyenHoangSon96 added this pull request to the merge queue Apr 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2025
@NguyenHoangSon96
Copy link
Contributor Author

Hi @blegesse-w
I added this PR to the merge queue, but monitor-ci-test was fail even though it was a success the last time
This happens a couple of times
Any idea?

@bednar
Copy link
Contributor

bednar commented Apr 15, 2025

The monitor-ci-tests is flaky, feel free to merge this PR.

@NguyenHoangSon96 NguyenHoangSon96 added this pull request to the merge queue Apr 15, 2025
Merged via the queue into master with commit 86ed246 Apr 15, 2025
6 checks passed
@NguyenHoangSon96 NguyenHoangSon96 deleted the feat/remove-unused-buttons branch April 15, 2025 12:29
@philjb philjb mentioned this pull request Jun 12, 2025
Open
philjb pushed a commit that referenced this pull request Jun 12, 2025
@NguyenHoangSon96 @philjb
feat(ui): remove notebook's unused buttons (#7008)
697e5d3 
* feat: remove notebook share button

* feat: remove notebook save button

* feat: show notebook buttons when CLOUD

* fix: prettier

* fix: review
@philjb philjb mentioned this pull request Jun 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bednar bednar bednar approved these changes

@blegesse-w blegesse-w blegesse-w approved these changes

@wdoconnell wdoconnell Awaiting requested review from wdoconnell

Assignees

@NguyenHoangSon96 NguyenHoangSon96

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove buttons for notebook versions in oss
4 participants
@NguyenHoangSon96 @blegesse-w @bednar @wdoconnell