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

feat: [M3-8016] - Add TagSelect to edit images drawer #10466

Merged
merged 13 commits into from
May 23, 2024

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented May 14, 2024

Description 📝

Adds a TagSelect to the edit image drawer based on the new field in the API.

Changes 🔄

  • Add TagSelect to edit image drawer
  • Update APIv4 Image type and factory with new tags field
  • Clean up ImageDrawer component, removing unused code

Target release date 🗓️

5/28

Preview 📷

Before After
Screenshot 2024-05-14 at 1 52 31 PM Screenshot 2024-05-14 at 1 52 19 PM

How to test 🧪

Prerequisites

  • Configure Cloud Manager to use the alpha API (e.g., via DevTools)

Verification steps

  • Navigate to the Images page
  • Click on the Action menu for an image and select 'Edit'
  • Verify tag select appears
  • Add/remove tags and click "Save Changes"
  • Re-open edit drawer and verify changes are reflected
  • Verify that there are no regressions in the Restore from Image drawer, which reuses this component

TODO

  • Create unit tests for ImageDrawer

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

@@ -42,15 +42,15 @@ export interface TagsInputProps {
/**
* Callback fired when the value changes.
*/
onChange: (selected: Item[]) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, this type is string | number

@bnussman-akamai bnussman-akamai added the Images Relating to Images label May 15, 2024
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review May 15, 2024 14:53
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner May 15, 2024 14:53
@hkhalil-akamai hkhalil-akamai requested review from jdamore-linode and cpathipa and removed request for a team May 15, 2024 14:53
@hkhalil-akamai hkhalil-akamai changed the title added: [M3-8016] - add TagSelect to edit images drawer added: [M3-8016] - Add TagSelect to edit images drawer May 15, 2024
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.

Tags are working well ✅

  • Found a few things 🐛
    • The Cancel button does not work for me currently
    • The loading state persists when the drawer is reopened
Screen.Recording.2024-05-15.at.12.31.52.PM.mov

We decided as a team that going forward that we'd like to keep drawers purpose very concise. So in this case, we could split up ImageDrawer into RestoreImageDrawer and EditImageDrawer. We don't need to do this now, but you can if you'd like to! It could also give us a chance to rewrite these forms to use react-hook-form, but again, not required

@mjac0bs mjac0bs changed the title added: [M3-8016] - Add TagSelect to edit images drawer feat: [M3-8016] - Add TagSelect to edit images drawer May 15, 2024
Copy link

github-actions bot commented May 15, 2024

Coverage Report:
Base Coverage: 81.65%
Current Coverage: 81.73%

@hkhalil-akamai
Copy link
Contributor Author

  • The Cancel button does not work for me currently
  • The loading state persists when the drawer is reopened

Good catch, fixed! ✅

@hkhalil-akamai
Copy link
Contributor Author

hkhalil-akamai commented May 16, 2024

We decided as a team that going forward that we'd like to keep drawers purpose very concise. So in this case, we could split up ImageDrawer into RestoreImageDrawer and EditImageDrawer. We don't need to do this now, but you can if you'd like to! It could also give us a chance to rewrite these forms to use react-hook-form, but again, not required

There is definitely room for cleanup in this component! Making this PR as WIP while I address this.

Update: I opened a separate PR to handle the clean up work in order to keep this PR more focused and avoid blocking the release of this feature. (cc @bnussman-akamai)

@hkhalil-akamai hkhalil-akamai marked this pull request as draft May 16, 2024 03:50
}

type CombinedProps = Props;

export type DrawerMode = 'closed' | 'create' | 'edit' | 'imagize' | 'restore';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'create' and 'imagize' modes of this drawer are no longer used, allowing for significant clean-up.

@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review May 20, 2024 15:34
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.

Tagging looks good! 🏷️ 🎉

I did notice this behavior shown in the video. Clearing a field and saving results in no change to the Image. I'd expect a validation error if I clear the label field. We can address this later

Screen.Recording.2024-05-20.at.5.28.38.PM.mov

@hkhalil-akamai hkhalil-akamai marked this pull request as draft May 21, 2024 16:37
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review May 21, 2024 16:37
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.

Great PR and cleanup!

left a couple code comments.

Also, to add to either this PR or the follow up ticket you created, when encountering an API error (ex: block PUT | https://api.dev.linode.com/v4/images/private requests), the error will persist when reopening the drawer.

Screen Shot 2024-05-21 at 15 58 54

Lastly, it's pretty strange to be able to add to add/edit tags in create/edit flow yet they are not visible anywhere in the UI when looking at the image list (prolly a follow up with UX would be good there)

Screen Shot 2024-05-21 at 16 01 22

packages/manager/src/features/Images/ImagesDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/ImagesDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Images/ImagesDrawer.tsx Outdated Show resolved Hide resolved
@hkhalil-akamai hkhalil-akamai merged commit 819356b into linode:develop May 23, 2024
17 of 18 checks passed
@hkhalil-akamai hkhalil-akamai deleted the edit-images-tags branch May 23, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Images Relating to Images
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants