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

fix: [M3-8929] - Unexpected gradient on Linode Detail with VPC interface #11289

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Nov 19, 2024

Description 📝

This PR fixes a wrong implementation of the Linode Detail VPC IP copy UI, introduced by #11172

It is not only a bad visual defect, but also prevents the ability to click on the action menu

Changes 🔄

  • remove <StyledGradientDiv>
  • improve implementation of copy tooltip
  • improve/cleanup styling

Preview 📷

Screenshot 2024-11-19 at 13 16 13


Screenshot 2024-11-19 at 14 37 30

How to test 🧪

Prerequisites

  • Have a Linode with a VPC interface

Reproduction steps

  • Navigate to the linode's detail
  • see issue with gradient

Verification steps

  • Navigate to the linode's detail
  • Confirm gradient fix
  • Confirm copy tooltip behavior

As an Author, I have considered 🤔

  • 👀 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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@abailly-akamai abailly-akamai added the Bug Fixes for regressions or bugs label Nov 19, 2024
@abailly-akamai abailly-akamai self-assigned this Nov 19, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review November 19, 2024 19:39
@abailly-akamai abailly-akamai requested a review from a team as a code owner November 19, 2024 19:39
@abailly-akamai abailly-akamai requested review from carrillo-erik, coliu-akamai and mjac0bs and removed request for a team November 19, 2024 19:39
Comment on lines 295 to 300
<StyledGradientDiv>
<CopyTooltip
copyableText
text={configInterfaceWithVPC.ipv4.vpc}
/>
</StyledGradientDiv>
<StyledCopyTooltip text={configInterfaceWithVPC.ipv4.vpc} />
{configInterfaceWithVPC.ipv4.vpc}
<Box sx={{ ml: 1, position: 'relative', top: 1 }}>
<StyledCopyTooltip text={configInterfaceWithVPC.ipv4.vpc} />
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the other copyable tooltips, we should make both the text and the icon copyable. Technically the gradient too but it's too slight to notice and we should probably remove it in the other places as well. No idea why the gradient broke here and not prior.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the other copyable tooltips, we should make both the text and the icon copyable.

This is a good point - agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

No idea why the gradient broke here and not prior.

It is most likely because the gradient is absolutely positioned, and if there is no immediate parent container with a relative position it will bubble up to to the next relative one, which can be any.

Long story short, there's no need for the gradient on this particular instance

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.

All good now.

Screenshot 2024-11-19 at 11 59 37 AM

Users can technically still click the action menu with the gradient bug, it just requires extreme precision. Thanks for the quick fix, @abailly-akamai!

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.

Thanks @abailly-akamai!

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing452 Passing2 Skipped80m 25s

Details

Failing Tests
SpecTest
update-stackscripts.spec.tsUpdate stackscripts » updates a StackScript

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/stackscripts/update-stackscripts.spec.ts"

@abailly-akamai abailly-akamai merged commit b9442d8 into linode:develop Nov 19, 2024
21 of 23 checks passed
Copy link

cypress bot commented Nov 19, 2024

Cloud Manager E2E    Run #6849

Run Properties:  status check failed Failed #6849  •  git commit b9442d8749: fix: [M3-8929] - Unexpected gradient on Linode Detail with VPC interface (#11289...
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6849
Run duration 27m 23s
Commit git commit b9442d8749: fix: [M3-8929] - Unexpected gradient on Linode Detail with VPC interface (#11289...
Committer Alban Bailly
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 452
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/stackscripts/update-stackscripts.spec.ts • 1 failed test

View Output Video

Test Artifacts
Update stackscripts > updates a StackScript Screenshots Video
Flakiness  cypress/e2e/core/linodes/linode-config.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Linode Config management > Mocked > Creates a new config using non-recommended settings and confirm the informational notices Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes for regressions or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants