feat: [M3-8023] - Refactor Image Upload and Add Tags #10484
Conversation
bnussman-akamai
requested review from
AzureLatte,
mjac0bs and
abailly-akamai
and removed request for
a team
|
Coverage Report: ✅ |
There was a problem hiding this comment.
I moved logic out of ImageUpload and into here to clean up ImageUpload as much as possible
There was a problem hiding this comment.
Lots of changes here. All of these changes were at my discretion. I will check with UX and make sure they are cool with everything.
- Batching logic isn't needed because you can only upload one image at a time
- I made the styles more obvious for when you drag and drop
- I moved the logic that makes the API request and upload request into
ImageUpload - Errors now show as a notice instead of a toast
- Added upload rate and estimated time
Screen.Recording.2024-05-17.at.3.58.58.PM.mov
There was a problem hiding this comment.
This is the primary component 🚨
- We now use
react-hook-form - There is now a submit button (it no longer submits when you drop the image)
- Removed one-off styles
- Added tags
- Removed unnessearcy logic that sends the user to login (this is handeled automatically elsewhere and shouldn't be needed)
- New UI based on Image Service Gen2 mockups
There was a problem hiding this comment.
This just contains utils for ImageUpload. I created this to try to keep ImageUpload clean
| const [label, setLabel] = React.useState<string>( | ||
| location?.state ? location.state.imageLabel : '' | ||
| ); | ||
| const [description, setDescription] = React.useState<string>( | ||
| location?.state ? location.state.imageDescription : '' | ||
| ); | ||
| const [isCloudInit, setIsCloudInit] = React.useState<boolean>(false); | ||
|
|
||
| const handleSetLabel = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
| setLabel(value); | ||
| }; | ||
|
|
||
| const handleSetDescription = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
| setDescription(value); | ||
| }; |
There was a problem hiding this comment.
All state is now handled by react-hook-form
|
@bnussman-akamai looks great! can you confirm all changes with UX before doing a deep dive review? |
packages/manager/src/components/LinodeCLIModal/LinodeCLIModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Love to see this simplification! 🧹
There was a problem hiding this comment.
Lots of great clean up and improvements here, thanks @bnussman-akamai. 🧼 ✨ The uploader styling with estimated time and upload rate all looks so much better!
Confirmed:
- The ability to upload images in Chrome, Firefox, Safari via both file explorer and drag and drop
- That styles looked good in both light and dark mode
- I encounter a confirmation modal if I navigate away from the upload page while upload is in progress
- Validation works by triggering various errors
- Tests pass
- Storybook story looks good - thanks for updating
- That the CLI command doesn't include tags
Left some other minor recommendations that are either typos or suggestions to improve UX. I can circle back to this if there are any further UX changes, but I'll approve in the meantime since overall functionality looks good so this doesn't get held up.
packages/manager/src/components/LinodeCLIModal/LinodeCLIModal.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/components/LinodeCLIModal/LinodeCLIModal.tsx
Outdated
Show resolved
Hide resolved
|
Great feedback @mjac0bs. I'm checking with UX on the |
There was a problem hiding this comment.
The code changes are a little too dense and touching so many areas it's pretty hard to review effectively. This is the kind of refactor that could've used a phased approach along with UX partnership before being put in a large PR.
All that being said it's a nice cleanup, featuring good improvements (upload CTA) and things are working well and "feel" good so I'll assume code quality and focus and a couple UI aspects:
Panels: (new change)
Region Select (opportunity to improve on existing)
Confirmation Toast:
👉 shows an closing icon but toast can't be dismissed, meanwhile when toast showing up when the image is available is dismissible
|
AlI changes have been reviewed and approved by UX 🎉 The "Upload Using Command Line" change was made @mjac0bs Going to check with UX on the region helper text and the two papers @abailly-akamai The broken Toast issue is out of scope of this work. |
|
Lol everything was kinda out of scope |
|
@abailly-akamai Noting that we do have a ticket for the broken toast close already (M3-8052). Thanks for checking with UX and confirming those improvements, @bnussman-akamai! |
|
Awesome rework - thanks for spending the time on this 👍 something funky with the pipeline again 🤔 |
|
@abailly-akamai UX confirmed they want to keep the separate papers. They provided some updated helper text for the region select. They requested it goes above the input instead of below |
Description 📝
Create Imagebutton 🔘 (previously, the upload would begin when the upload was selected)Preview 📷
How to test 🧪
Note
Don't have an Image to upload? This is how I generate a gziped image:
gzip alpine-standard-3.19.1-x86_64.iso.gzAs an Author I have considered 🤔