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: [M3-8157] - Clean up images drawer #10487

Merged
merged 33 commits into from
Jun 11, 2024
Merged

refactor: [M3-8157] - Clean up images drawer #10487

merged 33 commits into from
Jun 11, 2024

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented May 20, 2024 •
edited
Loading

Description 📝

Apply our latest coding standards to ImagesDrawer including:

  • Separating ImagesDrawer into EditImageDrawer and RestoreImageDrawer
  • Using react-hook-form instead of manually managing form state

Changes 🔄

File Major Changes
api-v4/src/images/images.ts Simplify updateImage function signature.
api-v4/src/images/types.ts New UpdateImagePayload type.
manager/src/features/Images/EditImageDrawer.test.tsx Tests for EditImageDrawer.
manager/src/features/Images/EditImageDrawer.tsx Dedicated component for editing images, extracted from previous ImagesDrawer. Also implemented react-hook-form for state management.
manager/src/features/Images/ImageRow.tsx Simplified prop drilling.
manager/src/features/Images/ImagesActionMenu.tsx Updates to Handlers interface - they are now passed the entire Image
manager/src/features/Images/ImagesDrawer.tsx Replaced with EditImageDrawer and RestoreImageDrawer
manager/src/features/Images/ImagesLanding.tsx Use new, separated drawer components. No longer stores drawer state.
manager/src/features/Images/RebuildImageDrawer.test.tsx Tests for RebuildImageDrawer.
manager/src/features/Images/RebuildImageDrawer.tsx Dedicated component for selecting images for rebuilding, extracted from previous ImagesDrawer. Also implemented react-hook-form for state management.
manager/src/features/Linodes/LinodesDetail/LinodeRebuild/RebuildFromImage.tsx Fix a bug where Image field was not being autopopulated from search params.
manager/src/features/Linodes/LinodesDetail/LinodesDetail.tsx Preserve search queries when redirecting.
manager/src/queries/images.ts New signature for updateImage function.

Preview 📷

No visual changes.

How to test 🧪

  • Verify Image Edit and Rebuild drawers work as expected:
    • Both drawers validate user inputs and display errors
    • Drawers reset when closing and re-opening
    • Submitting the Rebuild drawer redirects to the Linode Rebuild dialog with the Image select pre-populated

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

@hkhalil-akamai hkhalil-akamai self-assigned this May 20, 2024
@hkhalil-akamai hkhalil-akamai changed the title rafactor: Clean up images drawer refactor: Clean up images drawer May 20, 2024
@hkhalil-akamai hkhalil-akamai mentioned this pull request May 20, 2024
Merged
13 tasks
@hkhalil-akamai hkhalil-akamai changed the title refactor: Clean up images drawer refactor: [M3-8157] - Clean up images drawer May 23, 2024
hkhalil-akamai
hkhalil-akamai commented May 23, 2024
View reviewed changes
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Linodes/LinodesDetail/LinodesDetail.tsx
Comment on lines +58 to +62
search: new URLSearchParams({
...queryParams,
[path]: 'true',
}).toString(),
}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to preserve the search query specifically for the rebuild path. I don't think it hurts to do so for all other paths.

@hkhalil-akamai hkhalil-akamai added Images Relating to Images and removed Work in Progress labels May 23, 2024
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review May 23, 2024 21:22
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner May 23, 2024 21:22
bnussman-akamai
bnussman-akamai reviewed May 24, 2024
View reviewed changes
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Still needs to review a bit more, but cleanup looks great!

packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented May 24, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 82.29%
Current Coverage: 82.29%

bnussman-akamai
bnussman-akamai approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Clean up looks great 🔥 Much needed!

packages/manager/src/features/Images/ImagesLanding.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 5, 2024
abailly-akamai
abailly-akamai reviewed Jun 6, 2024
View reviewed changes
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 the cleanup! Nice to see react form here 🎉

Left a few comments to improve a couple things in the drawers

packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/EditImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/RebuildImageDrawer.tsx Show resolved Hide resolved
abailly-akamai
abailly-akamai approved these changes Jun 10, 2024
View reviewed changes
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.

Thx for the changes!

Approving pending CI passing

@hkhalil-akamai hkhalil-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jun 11, 2024
@hkhalil-akamai hkhalil-akamai merged commit f902075 into linode:develop Jun 11, 2024
18 checks passed
@hkhalil-akamai hkhalil-akamai deleted the refactor-images-drawer branch June 11, 2024 16:45
This was referenced Jun 24, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abailly-akamai abailly-akamai abailly-akamai approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

@jaalah-akamai jaalah-akamai Awaiting requested review from jaalah-akamai jaalah-akamai is a code owner automatically assigned from linode/frontend

@dwiley-akamai dwiley-akamai Awaiting requested review from dwiley-akamai

Assignees

@hkhalil-akamai hkhalil-akamai

Labels
Approved Multiple approvals and ready to merge! Images Relating to Images
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hkhalil-akamai @bnussman-akamai @abailly-akamai