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

Refactor BYON images table #1506

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Refactor BYON images table #1506

merged 1 commit into from
Jul 17, 2023

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Jul 12, 2023 •
edited

Closes #1432

Description

  1. Refactor the whole table, using the generic table component
  2. use useFetchState hook to get BYON images
  3. Refactor the usage of the modals. (as for the refactor of the contents in the modals, it will be solved in Update all text items and field labels in modals #1433)
Screenshot 2023-07-12 at 10 47 57 AM

How Has This Been Tested?

  1. Go to the settings -> Notebook image settings
  2. Create a notebook image, and try to play around with it, like editing, deleting, toggle the enablement... make sure nothing is broken

Test Impact

TBD

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@DaoDaoNoCode
Copy link
Member Author

@vconzola Can you check the changes as it's shown in the screenshot?

@DaoDaoNoCode DaoDaoNoCode linked an issue Jul 12, 2023 that may be closed by this pull request
Update all text items and field labels on Notebook images table #1432
Closed
@vconzola
Copy link

@DaoDaoNoCode Does this PR address including out-of-box images in the table - it doesn't look like it does? Will that be handled in a follow-up PR? Also, I don't see this spelled out in the issue requirements, but since we're not including click/drag yet, would it be a simple change (" one liner") to add sorting to the Enable column? That would allow users to see all of their enabled images at the top.

andrewballantyne
andrewballantyne reviewed Jul 12, 2023
View reviewed changes
frontend/src/pages/BYONImages/tableData.tsx
{
field: 'enable',
label: 'Enable',
sortable: false,
Copy link
Member

Choose a reason for hiding this comment

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

Actually, it should be a one liner @vconzola haha.

manaswinidas
manaswinidas reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

My only concerns are regarding pagination, how are we handling 20+ images without pagination?

backend/src/types.ts
name: string;
url: string;
description?: string;
// FIXME: This shouldn't be a user defined value consumed from the request payload but should be a controlled value from an authentication middleware.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this taken care of already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, I believe this part will be taken care of after we support OOTB images

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should be removing this comment then. If we are, we should be tracking it somewhere. Are we already tracking this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, i think we should just keep the comment here until we get to refactor the backend in phase 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the comment then, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry my bad, I thought I didn't remove the comment 😞 I will add it back in the following PR, thanks!

manaswinidas
manaswinidas reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

@openshift-ci openshift-ci bot merged commit 2c849f3 into opendatahub-io:f/byon Jul 17, 2023
5 checks passed
@DaoDaoNoCode
Copy link
Member Author

@vconzola The sorting or Enable column will be addressed along with this issue #1388 because we need to make some changes to the Enable toggle.

@DaoDaoNoCode
Copy link
Member Author

After the discussion with @vconzola , we are trying to keep the table as it was before - which means we will not add the pagination for now.

@DaoDaoNoCode DaoDaoNoCode mentioned this pull request Jul 17, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewballantyne andrewballantyne andrewballantyne left review comments

@manaswinidas manaswinidas manaswinidas left review comments

@Gkrumbach07 Gkrumbach07 Awaiting requested review from Gkrumbach07

@lucferbux lucferbux Awaiting requested review from lucferbux

Assignees

@manaswinidas manaswinidas

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update all text items and field labels on Notebook images table
4 participants
@DaoDaoNoCode @vconzola @andrewballantyne @manaswinidas