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.

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

Move disk source outside the add disk modal dialog #2061

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

avivtur
Copy link
Member

@avivtur avivtur commented Jun 27, 2024

📝 Description

Changing the Add disk button to a flyout-dropdown to select the disk source from outside the add disk dialog.

🎥 Demo

After:

reorganize-disk-modal.mp4

@openshift-ci openshift-ci bot requested review from metalice and tnisan June 27, 2024 11:52
@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Jun 27, 2024
<MenuContent>
<MenuList>
{options?.map(({ description, groupLabel, items }) => {
const isFlyout = items?.length > 1;
Copy link
Member

Choose a reason for hiding this comment

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

no need its only used once

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do this, I thought it would be more readable like this when the condition has context to it

</Menu>
);

return (
Copy link
Member

Choose a reason for hiding this comment

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

i understand why menu and toggle are here, as nested components, as they are deeply coupled, but still consider exporting them to new files and giving them the right Capital naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Extracting those to separated components is not working well for the MenuContainer as it expects a ReactElement type which have children:
Screenshot - 2024-06-30T153533 466

@avivtur avivtur force-pushed the move-disk-source branch from c5645b4 to d697000 Compare June 30, 2024 12:41
@avivtur avivtur force-pushed the move-disk-source branch from d697000 to 2992062 Compare July 4, 2024 11:25
@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Jul 17, 2024
Copy link
Contributor

openshift-ci bot commented Jul 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avivtur, metalice

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-merge-bot openshift-merge-bot bot merged commit d5e7d3d into kubevirt-ui:main Jul 17, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is something we want to fix lgtm Passed code review, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants