feat: [M3-7144] - MNTP Dialog Updates for DC-specific pricing by abailly-akamai · Pull Request #9692 · linode/manager

abailly-akamai · 2023-09-18T15:55:00Z

Description 📝

This PR improves the copy and typography in the Network Transfer Display Dialog.

Major Changes 🔄

Modifies copy
Adds header tooltips
Modifies size of typography elements

Preview 📷

Before	After

How to test 🧪

Setup

Enable the DC-Specific Pricing flag
Enable MSW

Testing Steps

Navigate to /linodes
Click on the "Monthly Network Transfer Pool" under the linodes table list
Confirm copy, tooltip behaviour and style changes
Modify the mock data and re-run the steps above (with 0, 1 and 2 or more transfer regions)

abailly-akamai · 2023-09-18T15:58:03Z

packages/manager/src/components/TooltipIcon.tsx

   * @default false
   */
-  leaveDelay?: boolean;
+  leaveDelay?: number;


This fixes an existing type issue on the component (unrelated to main PR purpose)

abailly-akamai · 2023-09-18T15:58:53Z

packages/manager/src/components/TooltipIcon.tsx

  return (
    <Tooltip
      classes={classes}
+      componentsProps={props.componentsProps}


Those were not picked up otherwise. Decided against passing {...props} to avoid regressions related to spreading everything (unknown HTML props etc)

abailly-akamai · 2023-09-18T16:01:13Z

packages/manager/src/components/TransferDisplay/TransferDisplayDialogHeader.tsx

+      />
+    </Typography>
+  );
+});


There's enough coverage of the feature to leave this new micro component without a unit test

mjac0bs

Copy and styling changes are mostly looking good. Tested different screen sizes (all looked good) and different combinations of pools (found a tooltip bug).

The couple things we'll want to fix

Did extending the divider to the edge of the dialog result in the horizontal scroll bar that is now appearing at the bottom of the dialog?

Screen.Recording.2023-09-18.at.9.45.33.AM.mov

When region_transfers is an empty array, the tooltip copy is incomplete. We'll need some logic there to end that sentence coherently. "except for those with data-center specific transfer", maybe?

packages/manager/.changeset/pr-9692-upcoming-features-1695054097678.md

abailly-akamai · 2023-09-18T18:01:46Z

@mjac0bs feedback addressed!

updated empty region_transfers tooltip to read: "The Global Pool includes transfer associated with active services in all regions." which to me is more relevant than adding copy that does not apply to the user.

All in all I am not liking this. I don't think Helper text should have conditional copy since it is an interactive element hidden by default. Perhaps this is something we can revisit at some point.

mjac0bs · 2023-09-18T20:27:49Z

which to me is more relevant than adding copy that does not apply to the user.

I think this is worth another opinion on too, probably Matthew's, because I worry we're contradicting ourselves here. The dialog already mentions the existence of regions calculating transfer differently, hinting at dc-specific pools.

I don't think Helper text should have conditional copy since it is an interactive element hidden by default.

Generally, I agree with this, but in this case, I think that the user is looking for content that is most relevant to them, so I don't have an issue with the conditional cases here. But we can revisit with UX, of course.

abailly-akamai · 2023-09-18T20:52:19Z

@mjac0bs how about replacing "in all regions" with "in your devices regions" - would work for both cases

mjac0bs · 2023-09-18T23:22:57Z

@mjac0bs how about replacing "in all regions" with "in your devices regions" - would work for both cases

This sounds good to me. 👍🏼 With the caveat that it would need to be "in your devices' regions", possessive, since regions belong to the devices.

coliu-akamai

So I think this is just the MSW being strange (the logic looks good to me and all the cases I've mentioned below have worked as expected at least once, so I'm unsure what's going on), but sometimes I get the following, and it's not really consistent when I get it:

Other than that, tested the following:

0 regions
1 region
2 regions
3 regions
and different widths + dark-mode and things looked good (except for the occasional flakiness like in the above screenshot but that's probably just the MSW?) 🎉 + left some very small comments

coliu-akamai · 2023-09-19T13:16:58Z

packages/manager/src/components/TransferDisplay/TransferDisplayDialog.tsx

        maxWidth="sm"
        onClose={onClose}
        open={isOpen}
+        sx={{}}


do we still need the sx here?

I also think we can remove it.

coliu-akamai · 2023-09-19T13:21:00Z

packages/manager/src/components/TransferDisplay/utils.ts

+ * @example formatRegionList(['Region 1', 'Region 2', 'Region 3']) // 'Region 1, Region 2 and Region 3'
+ * @example formatRegionList(['Region 1, Region 2']) // 'Region 1 and Region 2'
+ * @example formatRegionList(['Region 1']) // 'Region 1'
+ * @example formatRegionList([]) // ''


super small, but worth writing a test case for this as well (the empty [] case) just to hit all the cases 😆

Seconding this. 👍🏼

sure thing, done

mjac0bs

This looks good. The horizontal scroll is gone and the tooltip copy updates dynamically with different regions (0, 1, 2, 3, 4). Also confirmed that regions will display as long as they're in region_transfers, even if the quota is 0, as we'd expect.

I can't replicate the issue that @coliu-akamai was seeing, but with a test case for that util's empty case, this should be good to go. (Ha, this was done as I was writing this. 🚢 )

mjac0bs · 2023-09-19T17:49:57Z

packages/manager/src/components/TransferDisplay/TransferDisplayDialog.tsx

        maxWidth="sm"
        onClose={onClose}
        open={isOpen}
+        sx={{}}


I also think we can remove it.

mjac0bs · 2023-09-19T18:04:23Z

packages/manager/src/components/TransferDisplay/utils.ts

+ * @example formatRegionList(['Region 1', 'Region 2', 'Region 3']) // 'Region 1, Region 2 and Region 3'
+ * @example formatRegionList(['Region 1, Region 2']) // 'Region 1 and Region 2'
+ * @example formatRegionList(['Region 1']) // 'Region 1'
+ * @example formatRegionList([]) // ''


Seconding this. 👍🏼

abailly-akamai · 2023-09-19T18:14:05Z

@coliu-akamai yeah have not been able to repro this either. Other fixes addressed!

coliu-akamai

Yeah not too sure what was causing that issue, but since all the tests are working as expected, I don't think it should be a problem! approving 🎉

abailly-akamai added 3 commits September 18, 2023 10:46

feat: [M3-7144] Transfer Pool Dialog initial changes

ff52867

feat: [M3-7144] Formatting all the things

9eab5b4

feat: [M3-7144] fix tests

744c89b

abailly-akamai self-assigned this Sep 18, 2023

abailly-akamai commented Sep 18, 2023

View reviewed changes

abailly-akamai marked this pull request as ready for review September 18, 2023 16:20

abailly-akamai requested a review from a team as a code owner September 18, 2023 16:20

abailly-akamai requested review from coliu-akamai and tyler-akamai and removed request for a team September 18, 2023 16:20

Added changeset: Network Transfer Dialog copy and typography updates

305485b

abailly-akamai requested a review from mjac0bs September 18, 2023 16:22

feat: [fix-M3-7144] add missing period

8c581c5

mjac0bs added the DC-Specific Pricing label Sep 18, 2023

mjac0bs changed the title ~~Feat: [M3-7144] Network Transfer Dialog Updates~~ feat: [M3-7144] - MNTP Dialog Updates for DC-specific pricing Sep 18, 2023

mjac0bs reviewed Sep 18, 2023

View reviewed changes

packages/manager/.changeset/pr-9692-upcoming-features-1695054097678.md Outdated Show resolved Hide resolved

feat: [fix-M3-7144] feedback

a1331db

feat: [fix-M3-7144] feedback

c876ffd

abailly-akamai requested a review from mjac0bs September 19, 2023 12:54

coliu-akamai reviewed Sep 19, 2023

View reviewed changes

feat: [fix-M3-7144] feedback 2

d51f42a

mjac0bs approved these changes Sep 19, 2023

View reviewed changes

coliu-akamai approved these changes Sep 19, 2023

View reviewed changes

mjac0bs added the Approved Multiple approvals and ready to merge! label Sep 19, 2023

abailly-akamai merged commit bc25cd6 into linode:develop Sep 19, 2023

cpathipa mentioned this pull request Sep 29, 2023

Release v1.104.0 - release → staging #9740

Merged

mjac0bs mentioned this pull request Oct 2, 2023

Release v1.104.0 - staging → master #9744

Merged

mjac0bs mentioned this pull request Oct 12, 2023

feat: [M3-7063] - Provide dynamic price error handling for DC-specific pricing #9660

Merged

20 tasks

mjac0bs mentioned this pull request Nov 9, 2023

change, test: [M3-7073, M3-7176] - Remove dcSpecificPricing and objDcSpecificPricing feature flags #9881

Merged

12 tasks

Conversation

abailly-akamai commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Major Changes 🔄

Preview 📷

How to test 🧪

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjac0bs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abailly-akamai commented Sep 18, 2023

Uh oh!

mjac0bs commented Sep 18, 2023

Uh oh!

abailly-akamai commented Sep 18, 2023

Uh oh!

mjac0bs commented Sep 18, 2023

Uh oh!

coliu-akamai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coliu-akamai Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai commented Sep 19, 2023

Uh oh!

coliu-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abailly-akamai commented Sep 18, 2023 •

edited

Loading

mjac0bs left a comment •

edited

Loading

coliu-akamai left a comment •

edited

Loading

coliu-akamai Sep 19, 2023 •

edited

Loading