-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: [M3-7036] DC specific transfer pool displays #9620
Conversation
352d7fc
to
dd698df
Compare
ef5e999
to
fd05029
Compare
@@ -77,7 +77,8 @@ const StyledLinearProgress = styled(LinearProgress, { | |||
backgroundColor: '#99ec79', | |||
}, | |||
'& .MuiLinearProgress-barColorPrimary': { | |||
backgroundColor: '#5ad865', | |||
// Increase contrast if we have a buffer bar | |||
backgroundColor: props.valueBuffer ? '#1CB35C' : '#5ad865', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case (confirmed with UX)
TransferContent
is the only component that use valueBuffer
as a secondary color so maybe this will have to be adjusted later if other components use it. All in all, valueBuffer
shouldn't really be used for this case, it's more of a "secondary progress" to be shown alongside the main entity progress, so it we refactor we should modify this component.
Also, a gradient color (green to orange/red when getting to a threshold) was not really doable using this progress bar for a variety of reasons. This is really a progress meant to be a progress bar, not showing static data.
used < quota ? (used / quota) * 100 : used === 0 ? 0 : 100; | ||
if (!dcSpecificPricing) { | ||
return <LegacyTransferDisplay spacingTop={spacingTop} />; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment about cleaning things up when we get rid of the flag
// This is a copy of the original TransferDisplay component, which is getting deprecated. | ||
// It can be safely deleted once the new TransferDisplay component | ||
// is fully rolled out and the dcSpecificPricing flag is cleaned up. | ||
// ====================================================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is untouched - this will be far easier to cleanup once we get rid of the flag, see https://github.com/linode/manager/pull/9620/files#diff-7f49ccfd7b579b9abee32cb02d4aee03eee285169a2d85a40b0d6f59e0035d44R33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may notice the lack of dcSpecificPricing
flag usage here. It is because this component is the new version of the component left in LegacyTransferDisplay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also notice the lack of dcSpecificPricing
flag usage here. It is because this component is the new version of the component left in LegacyTransferDisplay
My bad @abailly-akamai, I gave you the wrong color spec for the light green on the Linode detail page. It should be #5BD865. That will match the green on the MNTP modal from the Linodes landing page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overall looking good. I played around with different mock values and found one issue with conditional checks.
In addition to the feedback that I left in-line, I'm wondering it would be best to not list the regional pools where there is no quota. Just thinking about how this list could grow in the future...
At the same time, it seems useful to show a regional transfer pool without any usage in the dialog view, especially while DC-specific pools are new.
This is likely an issue for posterity/our future selves once there are more DC-specific pools to worry about.
...res/Linodes/LinodesDetail/LinodeNetworking/NetworkingSummaryPanel/NetworkingSummaryPanel.tsx
Outdated
Show resolved
Hide resolved
...res/Linodes/LinodesDetail/LinodeNetworking/NetworkingSummaryPanel/NetworkingSummaryPanel.tsx
Outdated
Show resolved
Hide resolved
...c/features/Linodes/LinodesDetail/LinodeNetworking/NetworkingSummaryPanel/NetworkTransfer.tsx
Show resolved
Hide resolved
packages/manager/src/components/TransferDisplay/TransferDisplayUsage.tsx
Show resolved
Hide resolved
packages/manager/src/components/TransferDisplay/TransferDisplayDialog.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/components/TransferDisplay/TransferDisplay.tsx
Outdated
Show resolved
Hide resolved
...res/Linodes/LinodesDetail/LinodeNetworking/NetworkingSummaryPanel/NetworkingSummaryPanel.tsx
Show resolved
Hide resolved
...c/features/Linodes/LinodesDetail/LinodeNetworking/NetworkingSummaryPanel/TransferContent.tsx
Show resolved
Hide resolved
421fe31
to
e72e63d
Compare
@mjac0bs thanks for the review - all addressed and ready for another pass when you get a chance
I am in favor of listing all for now (we have very few) as it will make it clear to the user they have one or more pools available When the list grows we'll probably have more UI pieces to revisit all together |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work on this and its various details! Aside from a couple very minor things I found on second pass, this is looking good to me. 🚀
If you get another approval today and want to merge right after that, I'd just appreciate a heads up so I can merge develop into my open OBJ PR and that can be used for DC-specific pricing UX testing.
Edit: Also, I'm going to restart the unit tests, but the failure was unrelated (databases) and passed locally.
Description 📝
This PR adds DC specific transfer pool information for Global and per Linode usage.
It conditionally renders information based on the
dcSpecificPricing
and the presence ofregion_transfers
data in both thelinodeTransfer
andaccountTransfer
API responses.Preview 📷
How to test 🧪
The three areas for testing are shown in the screenshots above
For those three areas, the reviewer should:
dcSpecificPricing
is off