feat: [UIE-8089] - DBaaS Resize GA#11040
Conversation
|
Coverage Report: ✅ |
cc97faf to
325cee2
Compare
There was a problem hiding this comment.
We can simply this isDatabasesGAEnabled ? 'Please select a plan or set the number of nodes.' : 'Please select a plan.'
There was a problem hiding this comment.
This was part of the old version, so I've excluded this change as well.
packages/manager/src/features/Databases/DatabaseDetail/DatabaseResize/DatabaseResize.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can we simplify this to something like this?
{summaryText ? (
<>
<StyledPlanSummarySpan>
Resized Cluster: {summaryText.plan}
</StyledPlanSummarySpan>{' '}
{isDatabasesGAEnabled && (
<span className={classes.summarySpanBorder}>{summaryText.basePrice}</span>
)}
<span className={isDatabasesGAEnabled ? classes.nodeSpanSpacing : ''}>
{summaryText.numberOfNodes} Node{summaryText.numberOfNodes > 1 ? 's' : ''}
{isDatabasesGAEnabled ? ' - HA ' : ': '}
</span>
{summaryText.price}
</>
) : (
<>
<StyledPlanSummarySpan>Resized Cluster:</StyledPlanSummarySpan>{' '}
{isDatabasesGAEnabled
? 'Please select a plan or set the number of nodes.'
: 'Please select a plan.'}
</>
)}
There was a problem hiding this comment.
@cpathipa Unfortunately, there isn't an easy way to simplify this since many of the smaller changes in the markup are hidden behind that flag so that they won't show without it.
I didn't include this in the my changes, but perhaps we could rework this in a future PR or refactor.
There was a problem hiding this comment.
@smans-akamai I would recommend addressing the tech debt in this component, as it has grown too large and will be difficult to maintain in the future. It should be broken down into smaller components, as its current state is not maintainable.
There was a problem hiding this comment.
@cpathipa There's an upcoming ticket to refactor the CreateDatabase and ResizeDatabase components since they share a lot similar features (ie. node selector, summary). The ticket is UIE-8082 and that work is set for the 10/28 release and will involve breaking down these similar features into smaller, shared components.
So we should be able to move ahead with this change and address the tech debt and maintainability for these 2 components with that upcoming ticket.
There was a problem hiding this comment.
we should prefer userEvent over fireEvent whenever possible: https://testing-library.com/docs/user-event/intro/#differences-from-fireevent
| fireEvent.click(selectedNodeRadioButton); | |
| await userEvent.click(selectedNodeRadioButton); |
There was a problem hiding this comment.
Thanks @bnussman-akamai! I've included this changes in the test file!
3ae308e to
f50d2cf
Compare
cpathipa
left a comment
There was a problem hiding this comment.
Changes look good to me! Approving pending tests and follow-up tickets for refactoring the DatabaseResize component.
There was a problem hiding this comment.
@smans-akamai I would recommend addressing the tech debt in this component, as it has grown too large and will be difficult to maintain in the future. It should be broken down into smaller components, as its current state is not maintainable.
There was a problem hiding this comment.
This file needs so serious work
- There is way too much state
- State like
summaryText,nodePricing, andshouldSubmitBeDisabledall seem like states that do not need to exist. They likely should beconsts that are derived from existing state. See https://medium.com/@noriller/react-state-x-derived-state-97ff47769914 - Fixing these unessesary states will hopefully allow us to remove the
useEffects. We are really trying to eliminate use ofuseEffectin Cloud Manager because they are bug prone and generally a sign of bad state management
- State like
There was a problem hiding this comment.
@bnussman-akamai
There's an upcoming ticket to refactor the CreateDatabase and ResizeDatabase components since they share a lot similar features (ie. node selector, summary). The ticket is UIE-8082 and that work is set for the 10/28 release and will involve breaking down these similar components into smaller shared components.
This should change how summaryText, and nodePricing are handled since those components share states and logic. I can include a note in the ticket to look into changing how the disabling behavior for the submit button is disabled in Resize, rather than using shouldSubmitBeDisabled in state.
|
@smans-akamai Looks like there is E2E test failing in resize-database.spec.ts relevant to this PR. Could you look into this ? |
f50d2cf to
6b2aa25
Compare
…e cluster_size and export the Engines type
6b2aa25 to
ebdf47c
Compare
cpathipa
left a comment
There was a problem hiding this comment.
Confirming on the failed e2e test resize-database.spec
|
Looks like failed test clone-linode.spec is flaky cc: @jdamore-linode |
|
After taking with @jdamore-linode internally confirming failed clone-linode.spec is flaky. |
Description 📝
DBaaS Resize GA
Changes 🔄
List any change relevant to the reviewer.
Target release date 🗓️
10/14/24
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply