-
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
change: [M3-7996] - DC Get Well - PlanSelection availability updates and consolidation #10387
Conversation
* | ||
* TODO: Cypress | ||
* Move this to cypress component testing once the setup is complete - see https://github.com/linode/manager/pull/10134 | ||
* |
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.
@jdamore-linode added a TODO for both since they are candidates, even if unrelated to this PR
}); | ||
}); | ||
}); | ||
}); |
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.
Tried to be as detailed as possible including comments so the the logic is very clear for people coming in to modify anything
), | ||
reason: | ||
'This region is currently unavailable. For help, open a support ticket.', | ||
tooltipWidth: 250, |
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.
To allow for copy not to wrap
theme.palette.mode === 'dark' | ||
? theme.color.grey6 | ||
: theme.color.disabledText, | ||
theme.palette.mode === 'dark' ? theme.color.grey6 : theme.color.grey1, |
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.
Those changes allow the tootip not to be "buried" in the disable state and show as active while the row isn't
Coverage Report: ❌ |
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.
- Verified changes against prod
- Extracting shared logic is a big improvement
- One code suggestion
Thanks for including E2Es. Approved pending review of one suggestion.
const { | ||
allDisabledPlans, | ||
hasDisabledPlans, | ||
hasMajorityOfPlansDisabled, | ||
plansForThisLinodeTypeClass, | ||
} = extractPlansInformation({ | ||
disableLargestGbPlans: flags.disableLargestGbPlans, | ||
disabledPlanTypes: [], | ||
plans: plansMap, | ||
regionAvailabilities, | ||
selectedRegionId, | ||
}); |
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.
Can we move this down into KubernetesPlanContainer
to save one level of prop drilling?
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.
@hkhalil-akamai yes, this was considered but both <PlanInformation />
& <PlanContainer />
consume data from this util so it has to sit here to avoid redundancy
const { | ||
allDisabledPlans, | ||
hasDisabledPlans, | ||
hasMajorityOfPlansDisabled, | ||
plansForThisLinodeTypeClass, | ||
} = extractPlansInformation({ | ||
disableLargestGbPlans: flags.disableLargestGbPlans, | ||
disabledPlanTypes, | ||
plans: plansMap, | ||
regionAvailabilities, | ||
selectedRegionId: selectedRegionID, | ||
}); |
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.
Same comment as above: can we move this down into PlanContainer
to save one level of prop drilling?
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.
same comment
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.
RegionSelect tooltip update ✅
For Linode, LKE, and Database plan panels:
- always show an "unavailable" banner (red) if the whole plan panel is disabled ✅
- always show a "limited availability" (yellow) notice if a plan panel has limited availability ✅
- always show a tooltip if <= half of plans have limited availability ✅
- never show a tooltip if more than half of plans have limited availability ✅
To confirm, we're totally moving away from Learn more about plans and availability.
even when only one plan is disabled?
This branch | Prod |
---|---|
// Dedicated CPU tab | ||
// Should be selected/open by default | ||
// Should have the limited availability notice | ||
// Should contains 4 plans (5 rows including the header row) |
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.
// Should contains 4 plans (5 rows including the header row) | |
// Should contain 4 plans (5 rows including the header row) |
(typo correction applies to the equivalent comment for each case)
</> | ||
), | ||
reason: | ||
'This region is currently unavailable. For help, open a support ticket.', |
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.
Are we moving away from the general convention of having open a support ticket
linkified?
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.
That's correct, no more links in tooltips from what I understand
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.
Functionality looks good as far as I was able to test!
<Link to={docsLink}>Learn more</Link> about our {planTypeLabel} plans. | ||
</StyledNoticeTypography> | ||
</Notice> | ||
) : null; |
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.
Should this component return null? Could we move the null
return to LimitedAvailabilityNotice
?
It is actually belongs here, can we do
if (!hasDisabledPlans) {
return null;
}
for readability
Description 📝
This PR brings the latest changes to the PlanSelection panels on both the Linode and Kubernetes creation flows.
The diff is a bit larger than expected considering the need to refactor this portion of the code to allow for easier customization in the future. Both flows share very similar logic for rendering plan selection data, and identical logic for filtering unavailable and limited availability plans.
The logic implemented in this PR for the displaying of notices, disabled status and related tooltips is derived from the new requirements shown in the screenshots below:
Changes 🔄
List any change relevant to the reviewer.
Target release date 🗓️
04/29/2024
Preview 📷
How to test 🧪
Prerequisites
Verification steps
RegionSelect tooltip update
Best way i found to get a disabled RegionSelect item is on a newly created account (PROD)
Plan Selection (both for Linode and Kubernetes flows)
rules:
As an Author I have considered 🤔
Check all that apply