-
Notifications
You must be signed in to change notification settings - Fork 361
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-7288] - AGLB create page with Actions buttons. #9825
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
d120b11
feat: [M3-7288] - AGLB create page with Actions buttons.
cpathipa e2a78e3
Merge remote-tracking branch 'origin/develop' into M3-7288
cpathipa 7f56fb9
Add configuration header section
cpathipa 0d77241
Add Actions buttons
cpathipa c767907
Render Regions as per the mockup
cpathipa 3d2dc73
Added changeset: AGLB create page with Actions buttons.
cpathipa aa2d981
Unit test coverage for LoadBalancerLabel
cpathipa a9ceab7
Unit test for LoadBalancerConfiguration
cpathipa 078df04
Update packages/manager/src/features/LoadBalancers/LoadBalancerCreateβ¦
cpathipa f8b765e
Update packages/manager/.changeset/pr-9825-upcoming-features-16980932β¦
cpathipa af618d6
PR - Feedback
cpathipa 4d4e84c
Merge branch 'M3-7288' of github.com:cpathipa/manager into M3-7288
cpathipa 47fea16
PR - feedback
cpathipa 19ccdaf
Update packages/manager/src/features/LoadBalancers/LoadBalancerCreateβ¦
cpathipa f529242
Update packages/manager/src/features/LoadBalancers/LoadBalancerCreateβ¦
cpathipa f5dd31f
PR - Feedback
cpathipa e42026a
Fix alignment for small screen
cpathipa fb9122c
Override breadcrumb label
cpathipa ea2f25b
Fix tests
cpathipa 949349c
Adjust actions buttons order for small screens.
cpathipa 6fd6386
Update packages/manager/src/features/LoadBalancers/LoadBalancerCreateβ¦
cpathipa 413b325
Rename file name - LoadBalancerLabel
cpathipa c50c029
Merge branch 'M3-7288' of github.com:cpathipa/manager into M3-7288
cpathipa f5930a6
simplify styles and use less grid
bnussman d1f4b08
Wrap stepper component for mobile view
cpathipa a7344f2
add real regions
bnussman 71e8ff1
Use util - convertToKebabCase
cpathipa 8aba807
Merge branch 'M3-7288' of github.com:cpathipa/manager into M3-7288
cpathipa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
5 changes: 5 additions & 0 deletions
5
packages/manager/.changeset/pr-9825-upcoming-features-1698093221284.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/manager": Upcoming Features | ||
--- | ||
|
||
Add AGLB create page with Actions buttons ([#9825](https://github.com/linode/manager/pull/9825)) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
67 changes: 67 additions & 0 deletions
67
.../manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerConfiguration.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { screen } from '@testing-library/react'; | ||
import userEvent from '@testing-library/user-event'; | ||
import React from 'react'; | ||
|
||
import { renderWithTheme } from 'src/utilities/testHelpers'; | ||
|
||
import { LoadBalancerConfiguration } from './LoadBalancerConfiguration'; | ||
|
||
describe('LoadBalancerConfiguration', () => { | ||
test('Should render Details content', () => { | ||
renderWithTheme(<LoadBalancerConfiguration />); | ||
expect( | ||
screen.getByText('TODO: AGLB - Implement Details step content.') | ||
).toBeInTheDocument(); | ||
expect( | ||
screen.queryByText( | ||
'TODO: AGLB - Implement Service Targets Configuration.' | ||
) | ||
).toBeNull(); | ||
expect( | ||
screen.queryByText('TODO: AGLB - Implement Routes Confiugataion.') | ||
).toBeNull(); | ||
expect(screen.getByText('Next: Service Targets')).toBeInTheDocument(); | ||
expect(screen.queryByText('Previous: Details')).toBeNull(); | ||
}); | ||
test('Should navigate to Service Targets content', () => { | ||
renderWithTheme(<LoadBalancerConfiguration />); | ||
userEvent.click(screen.getByTestId('service-targets')); | ||
expect( | ||
screen.getByText('TODO: AGLB - Implement Service Targets Configuration.') | ||
).toBeInTheDocument(); | ||
expect( | ||
screen.queryByText('TODO: AGLB - Implement Details step content.') | ||
).toBeNull(); | ||
expect( | ||
screen.queryByText('TODO: AGLB - Implement Routes Confiugataion.') | ||
).toBeNull(); | ||
expect(screen.getByText('Next: Routes')).toBeInTheDocument(); | ||
expect(screen.getByText('Previous: Details')).toBeInTheDocument(); | ||
expect(screen.queryByText('Previous: Service Targets')).toBeNull(); | ||
}); | ||
test('Should navigate to Routes content', () => { | ||
renderWithTheme(<LoadBalancerConfiguration />); | ||
userEvent.click(screen.getByTestId('service-targets')); | ||
userEvent.click(screen.getByTestId('routes')); | ||
expect( | ||
screen.queryByText('TODO: AGLB - Implement Details step content.') | ||
).toBeNull(); | ||
expect( | ||
screen.queryByText( | ||
'TODO: AGLB - Implement Service Targets Configuration.' | ||
) | ||
).toBeNull(); | ||
expect( | ||
screen.getByText('TODO: AGLB - Implement Routes Confiugataion.') | ||
).toBeInTheDocument(); | ||
expect(screen.getByText('Previous: Service Targets')).toBeInTheDocument(); | ||
}); | ||
test('Should be able to go previous step', () => { | ||
renderWithTheme(<LoadBalancerConfiguration />); | ||
userEvent.click(screen.getByTestId('service-targets')); | ||
userEvent.click(screen.getByText('Previous: Details')); | ||
expect( | ||
screen.getByText('TODO: AGLB - Implement Details step content.') | ||
).toBeInTheDocument(); | ||
}); | ||
}); |
44 changes: 44 additions & 0 deletions
44
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerConfiguration.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import Stack from '@mui/material/Stack'; | ||
import * as React from 'react'; | ||
|
||
import { Paper } from 'src/components/Paper'; | ||
import { Typography } from 'src/components/Typography'; | ||
import { VerticalLinearStepper } from 'src/components/VerticalLinearStepper/VerticalLinearStepper'; | ||
|
||
export const configurationSteps = [ | ||
{ | ||
content: <div>TODO: AGLB - Implement Details step content.</div>, | ||
handler: () => null, | ||
label: 'Details', | ||
}, | ||
{ | ||
content: <div>TODO: AGLB - Implement Service Targets Configuration.</div>, | ||
handler: () => null, | ||
label: 'Service Targets', | ||
}, | ||
{ | ||
content: <div>TODO: AGLB - Implement Routes Confiugataion.</div>, | ||
handler: () => null, | ||
label: 'Routes', | ||
}, | ||
]; | ||
|
||
export const LoadBalancerConfiguration = () => { | ||
return ( | ||
<Paper> | ||
<Typography | ||
sx={(theme) => ({ marginBottom: theme.spacing(2) })} | ||
variant="h2" | ||
> | ||
Configuration -{' '} | ||
</Typography> | ||
<Stack spacing={1}> | ||
<Typography> | ||
A Configuration listens on a port and uses Route Rules to forward | ||
request to Service Target Endpoints | ||
</Typography> | ||
<VerticalLinearStepper steps={configurationSteps} /> | ||
</Stack> | ||
</Paper> | ||
); | ||
}; |
52 changes: 50 additions & 2 deletions
52
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerCreate.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 49 additions & 0 deletions
49
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerLabel.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import React from 'react'; | ||
|
||
import { renderWithTheme } from 'src/utilities/testHelpers'; | ||
|
||
import { LoadBalancerLabel } from './LoadBalancerLabel'; | ||
|
||
describe('LoadBalancerLabel', () => { | ||
it('should render the component with a label and no error', () => { | ||
const labelFieldProps = { | ||
disabled: false, | ||
errorText: '', | ||
label: 'Load Balancer Label', | ||
onChange: jest.fn(), | ||
value: 'Test Label', | ||
}; | ||
|
||
const { getByTestId, queryByText } = renderWithTheme( | ||
<LoadBalancerLabel error="" labelFieldProps={labelFieldProps} /> | ||
); | ||
|
||
const labelInput = getByTestId('textfield-input'); | ||
const errorNotice = queryByText('Error Text'); | ||
|
||
expect(labelInput).toBeInTheDocument(); | ||
expect(labelInput).toHaveAttribute('placeholder', 'Enter a label'); | ||
expect(labelInput).toHaveValue('Test Label'); | ||
expect(errorNotice).toBeNull(); | ||
}); | ||
|
||
it('should render the component with an error message', () => { | ||
const labelFieldProps = { | ||
disabled: false, | ||
errorText: 'This is an error', | ||
label: 'Load Balancer Label', | ||
onChange: jest.fn(), | ||
value: 'Test Label', | ||
}; | ||
|
||
const { getByTestId, getByText } = renderWithTheme( | ||
<LoadBalancerLabel error="Error Text" labelFieldProps={labelFieldProps} /> | ||
); | ||
|
||
const labelInput = getByTestId('textfield-input'); | ||
const errorNotice = getByText('This is an error'); | ||
|
||
expect(labelInput).toBeInTheDocument(); | ||
expect(errorNotice).toBeInTheDocument(); | ||
}); | ||
}); |
36 changes: 36 additions & 0 deletions
36
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerLabel.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import * as React from 'react'; | ||
|
||
import { Notice } from 'src/components/Notice/Notice'; | ||
import { Paper } from 'src/components/Paper'; | ||
import { TextField, TextFieldProps } from 'src/components/TextField'; | ||
|
||
interface LabelProps { | ||
error?: string; | ||
labelFieldProps: TextFieldProps; | ||
} | ||
|
||
export const LoadBalancerLabel = (props: LabelProps) => { | ||
const { error, labelFieldProps } = props; | ||
|
||
return ( | ||
<Paper | ||
sx={{ | ||
flexGrow: 1, | ||
width: '100%', | ||
}} | ||
data-qa-label-header | ||
> | ||
{error && <Notice text={error} variant="error" />} | ||
<TextField | ||
data-qa-label-input | ||
disabled={labelFieldProps.disabled} | ||
errorText={labelFieldProps.errorText} | ||
label="Load Balancer Label" | ||
noMarginTop | ||
onChange={() => labelFieldProps.onChange} | ||
placeholder="Enter a label" | ||
value={labelFieldProps.value} | ||
/> | ||
</Paper> | ||
); | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We can clean this up a little by doing this instead and with the other tests
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.
IMO, I don't see any significant benefits in choosing between using 'screen' methods
and destructuring the 'render' result. Both approaches are concise and easy to read, and I have
no strong opinions on which one to use. It largely depends on personal preference.
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.
While screen works great, I also have a deep fear that it one day will cause issues. It scares me that it's just some magic global that happens to have the correct scope when used in a test. I have no idea how it is implemented but I generally prefer destructuring the 'render' result because it seems more direct and less "magic" to me. I'm sure either method is fine but just wanted to provide another perspective.
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.
Had a quick look in into testing-library documentation, looks like they are recommending
screen
methods to find DOM elements (Didn't do in-depth research) and it also supports multiple frameworks.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.
Very interesting, good find. π