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

Add ExpandSectionWithSwitch component #1583

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

pcbailey
Copy link
Member

📝 Description

This PR:

  • Adds the ExpandSectionWithSwitch component, which will be used by the kernel same-page merging and auto-compute CPU limits features on the Settings page
  • Adds the isIndented and isDisabled options to the ExpandSection component
  • Adds the isIndented prop to all expandable sections on the Settings page

🎥 Screenshots

ExpandSectionWithSwitch example (enabled and switch on)

expand-section-with-switch-example2023-10-12_09-09

ExpandSectionWithSwitch example (disabled)

expand-section-with-switch-disabled-example-2023-10-12_09-09

Settings page before (without indents)

settings-page-expanded-no-indent-2023-10-12_09-08

Settings page after (with indents)

settings-page-expanded-2023-10-12_09-04

@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Oct 12, 2023
@pcbailey pcbailey force-pushed the expand-section-with-switch branch 2 times, most recently from 940c603 to 73f255e Compare October 12, 2023 19:13
@upalatucci
Copy link
Member

for me is lgtm. @hstastna

Copy link

@hstastna hstastna 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 adding isIndented prop is unnecessary, as I think all that content should have already been intended nicely and by default, without any extra prop. I mean indentation is great, I'd leave it there by default, just adding a prop to do so is unnecessary. Otherwise 👍🏽 ❇️

Just a note that according to the design changes, KSM feature won't need this component anymore.

@pcbailey
Copy link
Member Author

pcbailey commented Oct 13, 2023

I think adding isIndented prop is unnecessary, as I think all that content should have already been intended nicely and by default, without any extra prop.

I updated it to be true by default in ExpandSection. That way it'll be available if needed, but otherwise we won't have to worry about setting it each time. Thanks for pointing that out!

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Oct 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hstastna, pcbailey

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 432d11b into kubevirt-ui:main Oct 13, 2023
8 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.

3 participants