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-8872] - Table and Chart Legend Improvements #11294

Merged
merged 9 commits into from
Nov 26, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Nov 20, 2024

Description 📝

We needed to reduce the spacing for our table headers to match our CDS as well as within our legend table.

Changes 🔄

  • Added a space between legend tooltip value and unit
  • Reduced the height of the table header across the board from 46 to 40 to match guidance from CDS
  • Removed additional padding for legends
  • Added ability to use built-in y-axis formatter

Target release date 🗓️

12/10

Preview 📷

Before After
Screenshot 2024-11-20 at 10 06 47 AM Screenshot 2024-11-20 at 10 06 37 AM
localhost_3000_linodes_67142831 (3) localhost_3000_linodes_67142831
localhost_3000_linodes_67142831 (2) localhost_3000_linodes_67142831 (1)

How to test 🧪

Reproduction steps

  • Observe style changes via prod and this branch
  • Linodes landing table
  • Linodes detail page has the network charts

Verification steps

  • Verify header changes look good across the board

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

@jaalah-akamai jaalah-akamai self-assigned this Nov 20, 2024
@jaalah-akamai jaalah-akamai marked this pull request as ready for review November 20, 2024 16:22
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner November 20, 2024 16:22
@jaalah-akamai jaalah-akamai requested review from mjac0bs and coliu-akamai and removed request for a team November 20, 2024 16:22
@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Nov 20, 2024

Will update branch soon...

@jaalah-akamai jaalah-akamai marked this pull request as draft November 21, 2024 15:10
@jaalah-akamai jaalah-akamai marked this pull request as ready for review November 21, 2024 21:19
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner November 21, 2024 21:19
@jaalah-akamai jaalah-akamai requested review from jdamore-linode and removed request for a team November 21, 2024 21:19
Copy link

Coverage Report:
Base Coverage: 86.83%
Current Coverage: 86.83%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 3 failing tests on test run #6 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
3 Failing451 Passing2 Skipped111m 14s

Details

Failing Tests
SpecTest
linode-storage.spec.tslinode storage tab » delete disk
linode-storage.spec.tslinode storage tab » add a disk
linode-storage.spec.tslinode storage tab » resize disk

Troubleshooting

Use this command to re-run the failing tests:

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

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.

Confirmed the spacing is improved and expected test coverage passes.

Screen.Recording.2024-11-25.at.2.23.46.PM.mov

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Nov 25, 2024
@nikhagra-akamai
Copy link
Contributor

@jaalah-akamai one thing seen in y-axis that for a very low max value when recharts generating all the y-axis ticks, some tick points are going to 4 decimal places. As you can see, values like 0.0075 or 0.0225 are not properly visible as y-axis tick. Can we add some kind of padding or formatter for the value?
Screenshot 2024-11-26 at 3 01 17 PM

@jaalah-akamai
Copy link
Contributor Author

I added the ability to use the built-in formatter, so you can apply whatever you need in that case.

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.

✅ styling changes in tables in graphs + across the board

thank you!

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 26, 2024
@jaalah-akamai jaalah-akamai merged commit afa6da9 into linode:develop Nov 26, 2024
20 of 23 checks passed
Copy link

cypress bot commented Nov 26, 2024

Cloud Manager E2E    Run #6886

Run Properties:  status check failed Failed #6886  •  git commit afa6da9f35: fix: [M3-8872] - Table and Chart Legend Improvements (#11294)
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6886
Run duration 30m 30s
Commit git commit afa6da9f35: fix: [M3-8872] - Table and Chart Legend Improvements (#11294)
Committer Jaalah Ramos
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
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 454
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/linodes/rebuild-linode.spec.ts • 1 failed test

View Output Video

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

View Output Video

Test Artifacts
switch linode state > powers off a linode from landing page Screenshots Video
Flakiness  firewalls/migrate-linode-with-firewall.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Migrate Linode With Firewall > migrates linode with firewall - real data 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! Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants