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-8937] - Content shifting on Linode Details summary graphs #11301

Merged

Conversation

bnussman-akamai
Copy link
Member

Description 📝

Quick PR to try to improve the content shifting when loading a Linode Details page. It is pretty bad right now 😖

  • Improved loading state so that there is less content shifting 🔄📈
  • Cleaned up styled components / one-off styles 🧹
  • Improved dark mode styles 🌑

Preview 📷

Before After
Improved loading state
Screen.Recording.2024-11-21.at.10.09.15.AM.mov
Screen.Recording.2024-11-21.at.10.09.34.AM.mov
Improved dark mode Screenshot 2024-11-21 at 10 07 47 AM Screenshot 2024-11-21 at 10 07 43 AM

How to test 🧪

  • Test the Linode Details page summary (graphs) section for style and functionality regressions 👀 📈 git

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

@bnussman-akamai bnussman-akamai added the UX/UI Changes for UI/UX to review label Nov 21, 2024
@bnussman-akamai bnussman-akamai self-assigned this Nov 21, 2024
@bnussman-akamai bnussman-akamai changed the title fix: Improve content shifting on Linode Details summary graphs fix: Content shifting on Linode Details summary graphs Nov 21, 2024
@bnussman-akamai bnussman-akamai changed the title fix: Content shifting on Linode Details summary graphs fix: [M3-8937] - Content shifting on Linode Details summary graphs Nov 21, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review November 21, 2024 18:54
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner November 21, 2024 18:54
@bnussman-akamai bnussman-akamai requested review from dwiley-akamai and coliu-akamai and removed request for a team November 21, 2024 18:54
Copy link

github-actions bot commented Nov 22, 2024

Coverage Report:
Base Coverage: 86.98%
Current Coverage: 86.94%

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.

For some reason, the graphs aren't rendering for me on this branch

Screen.Recording.2024-11-22.at.4.18.01.PM.mov

@@ -40,15 +40,11 @@ import type { Item } from 'src/components/EnhancedSelect/Select';
setUpCharts();

interface Props {
isBareMetalInstance: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about removing the bare metal checks in this file 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The API team removed all of their bare metal related code in their PR #6768 a few months ago, so I think it's fine. We might want to do a similar clean up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, in that case I agree it should be fine and we should have a ticket for codebase clean-up as well

@bnussman-akamai
Copy link
Member Author

@dwiley-akamai I think this change caused that when I merged in the latest develop. I made a change reverting that change (and had to update a few cypress tests)

@bnussman-akamai bnussman-akamai requested a review from a team as a code owner November 22, 2024 22:02
@bnussman-akamai bnussman-akamai requested review from jdamore-linode and removed request for a team November 22, 2024 22:02
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.

Graph boxes don't jump in size once the graph fully loads ✅

One thing I did notice is at the smallest viewport width, upon first loading the page, the placement of the graphs is wonky (in prod, they stack as expected):

Screenshot

Screenshot 2024-11-22 at 5 35 44 PM

@@ -40,15 +40,11 @@ import type { Item } from 'src/components/EnhancedSelect/Select';
setUpCharts();

interface Props {
isBareMetalInstance: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, in that case I agree it should be fine and we should have a ticket for codebase clean-up as well

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

✅ Responsive Behavior
✅ Works with other charts (Cloud Pulse)
✅ Dark Mode 🔥
👀 Was not noticing what @dwiley-akamai was observing

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Nov 25, 2024
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Noticing the same as Dajahi + sometimes the graphs don't adjust to the screen's width when going from smaller to larger widths. I'm on chrome - will try testing on other browsers

linode-graphs.mov

@bnussman-akamai
Copy link
Member Author

@coliu-akamai @dwiley-akamai Good catch. I pushed 29c492b to hopefully fix that issue. Let me know if that worked

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

worked for me :thxbanks: 🚀

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing454 Passing2 Skipped117m 0s

Details

Failing Tests
SpecTest
backup-linode.spec.tslinode backups » can capture a manual snapshot

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/backup-linode.spec.ts"

@bnussman-akamai bnussman-akamai merged commit f16fa7e into linode:develop Nov 25, 2024
22 of 23 checks passed
Copy link

cypress bot commented Nov 26, 2024

Cloud Manager E2E    Run #6881

Run Properties:  status check failed Failed #6881  •  git commit f16fa7ed4b: fix: [M3-8937] - Content shifting on Linode Details summary graphs (#11301)
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6881
Run duration 32m 43s
Commit git commit f16fa7ed4b: fix: [M3-8937] - Content shifting on Linode Details summary graphs (#11301)
Committer Banks Nussman
View all properties for this run ↗︎

Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 2
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 453
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/linodes/rebuild-linode.spec.ts • 2 failed tests

View Output Video

Test Artifacts
rebuild linode > rebuilds a linode from Community StackScript Screenshots Video
rebuild linode > rebuilds a linode from Account StackScript Screenshots Video
Flakiness  linodes/clone-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
clone linode > can clone a Linode from Linode details page Screenshots Video
Flakiness  nodebalancers/nodebalancers-create-in-complex-form.spec.ts • 1 flaky test

View Output Video

Test Artifacts
create NodeBalancer to test the submission of multiple nodes and multiple configs > displays errors during adding new config Screenshots Video

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! UX/UI Changes for UI/UX to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants