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

change: [M3-6843] - Move manual snapshot error message to snapshot confirmation dialog #10791

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Aug 16, 2024

Description πŸ“

Moves the manual snapshot error message from the Linode Backups page to the snapshot's confirmation dialog

Target release date πŸ—“οΈ

n/a

Preview πŸ“·

Note - three potential options for displaying the error message - see "Other notes/questions" for more

Before After
image imageimageimage

How to test πŸ§ͺ

  • Have a busy linode (a linode that is already creating a snapshot, shutting down, turning on, etc) or a linode with no disk and no config and try to create a snapshot
  • Confirm error message is now in the dialog
  • Confirm that error message clears when closing and then reopening dialog

Other notes/questions

  • several potential ways to display the snapshot error in the confirmation dialog
    • the first (current) one matches the rest most of the confirmation dialogs
    • other two more closely resemble how the error message used to appear.
    • I'm thinking that the first option is probably the way to go for consistency? but can change it if the other options are preferred
  • should we add a toast for disk deletion initiation? [See internal ticket comments for context]

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

@coliu-akamai coliu-akamai self-assigned this Aug 16, 2024
import { BackupsPlaceholder } from './BackupsPlaceholder';
import { BackupTableRow } from './BackupTableRow';
Copy link
Contributor Author

@coliu-akamai coliu-akamai Aug 16, 2024

Choose a reason for hiding this comment

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

these are just eslint changes (same with line 75 of CaptureSnapshot.tsx)

@@ -5,7 +5,7 @@ import { ConfirmationDialog } from 'src/components/ConfirmationDialog/Confirmati
import { Typography } from 'src/components/Typography';

interface Props {
error?: string;
error: string | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no change in type/functionality of this prop, just made it non optional bc the one place we use this component now passes in an error

@coliu-akamai coliu-akamai changed the title change: [M3-6843] - Move manual snapshot error message to snapshot confirmation dialogue change: [M3-6843] - Move manual snapshot error message to snapshot confirmation dialog Aug 16, 2024
@coliu-akamai coliu-akamai marked this pull request as ready for review August 16, 2024 16:03
@coliu-akamai coliu-akamai requested a review from a team as a code owner August 16, 2024 16:03
@coliu-akamai coliu-akamai requested review from dwiley-akamai, hkhalil-akamai and mjac0bs and removed request for a team August 16, 2024 16:03
Copy link

github-actions bot commented Aug 16, 2024

Coverage Report: βœ…
Base Coverage: 82.62%
Current Coverage: 82.62%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Tested and looks good! Left one suggestion about using TransitionProps instead of useEffect.

several potential ways to display the snapshot error in the confirmation dialog

I agree with the using the first way for consistency, but we could check with UX if you feel like the other pattern is more appropriate. It may be out of scope of this ticket though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this test!!!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Good question about the positioning of the error notice. We're inconsistent with this.

Searching <ConfirmationDialog in the codebase, there are several that places we let the ConfirmationDialog format it below (as shown here) and several other places where we put the error first. (ConfirmTransferCancelDialog, ImagesLanding, DeletionDialog are a few).

It does seem like we go with Option 1 the most and for that reason, I would suggest sticking to that here... but it might be worth confirming with UX if we want to continue to go that route or make this consistent with one approach throughout our confirmation dialogs (in a separate ticket). Personally, I find it difficult to read, especially in dark mode, because the error message is so small and the color contrast is poor.

Another note: This Linode has nothing to backup, please add a disk and/or and a config is grammatically incorrect and really should be fixed on the backend. Could we let the API team know about this? (They may be fine with just a ticket since it looks like it's an error message coming from them and not some other service through APIv4.)

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Aug 16, 2024

Created internal ticket [M3-8453] + will add to frontend cafe discussion too!
Will reach out to API about error message - internal ticket created

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Code review βœ…
Unit test passes βœ…
Error message appears in confirmation dialog now βœ…

My personal preference would be for the second of the three possible display options (error notice dialog body text), but connecting with UX to set a convention would be great

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Aug 16, 2024
@coliu-akamai coliu-akamai merged commit bb375ff into linode:develop Aug 19, 2024
19 checks passed
@coliu-akamai coliu-akamai deleted the m3-6843 branch August 27, 2024 16:40
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants