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

refactor: [M3-8143] - Improve PowerActionsDialog #10667

Merged
merged 8 commits into from
Jul 16, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Jul 10, 2024

Description 📝

This PR helps improve the janky loading state of the PowerActionsDialog by removing the flash of content when a dialog or drawer is rendered. The user is now shown helper text in the Reboot Linode dialog above the config select component.

Changes 🔄

List any change relevant to the reviewer.

  • Remove redundant uses of useLinodeQuery.
  • Pass the linodeLabel prop to child component to eliminate the flash of content.
  • Replace default exports with named exports.
  • Introduce helper text for the config select component in Reboot Linode dialog.

Target release date 🗓️

07/22/2024

Preview 📷

Before After
config-before config-after
PowerActionsDialog-before.mov
PowerActionsDialog-after.mov

How to test 🧪

Prerequisites

(How to setup test environment)

  • Either:
    • Have at least two linode instances with several configurations saved.
    • Turn on the MSW.

Verification steps

(How to verify changes)

  • Start with visiting the Linodes landing page.
  • Click on the LinodeActionMenu and select any of the options.
  • Observe that the flash of content has been resolved.
  • Verify that the helper text is rendered below the config select in the Reboot Linode dialog.
  • Verify that no regressions were introduced with these changes.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@carrillo-erik carrillo-erik self-assigned this Jul 10, 2024
@carrillo-erik carrillo-erik requested a review from a team as a code owner July 10, 2024 17:13
@carrillo-erik carrillo-erik requested review from bnussman-akamai and AzureLatte and removed request for a team July 10, 2024 17:13
Copy link

github-actions bot commented Jul 11, 2024

Coverage Report:
Base Coverage: 82.48%
Current Coverage: 82.48%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks good and other changes are safe. Left a comment for a fix in the reboot modal that will improve the UX

@@ -96,7 +97,7 @@ export const LinodeRebuildDialog = (props: Props) => {
maxWidth="md"
onClose={onClose}
open={open}
title={`Rebuild Linode ${linode?.label ?? ''}`}
title={`Rebuild Linode ${linodeLabel ?? ''}`}
>
<StyledDiv>
{unauthorized && <LinodePermissionsError />}
Copy link
Contributor

Choose a reason for hiding this comment

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

This components needs its form state reset on either close or open - especially since the Config Select clearable prop does not seem to any effect. Ex: select a config and close the modal, open it back up and the previously selected config will be selected and unclearable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abailly-akamai This issue was also occurring with onSubmit(), both cases for the state reset have been addressed. Also, the isClearable prop now works as expected.

@carrillo-erik carrillo-erik requested a review from a team as a code owner July 15, 2024 16:23
@carrillo-erik carrillo-erik requested review from jdamore-linode and removed request for a team July 15, 2024 16:23
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the changes - confirmed the modal select is not clearable and reset properly. ✅

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Jul 16, 2024
@carrillo-erik carrillo-erik merged commit 3a6584c into linode:develop Jul 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants