Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: [M3-6889] CreateCluster styles #9835

Merged
merged 2 commits into from
Oct 25, 2023
Merged

fix: [M3-6889] CreateCluster styles #9835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce957af
Fix styles and clean up component
abailly-akamai Oct 24, 2023
2f9cd45
Added changeset: CreateCluster form label styles and layout
abailly-akamai Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-9835-fixed-1698158982152.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

CreateCluster form label styles and layout ([#9835](https://github.com/linode/manager/pull/9835))
58 changes: 58 additions & 0 deletions packages/manager/src/features/Kubernetes/CreateCluster/CreateCluster.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { Theme } from '@mui/material/styles';
import { makeStyles } from '@mui/styles';

export const useStyles = makeStyles((theme: Theme) => ({
inputWidth: {
'& .react-select__menu': {
maxWidth: 440,
},
maxWidth: 440,
},
regionSubtitle: {
'& .MuiInput-root': {
maxWidth: 440,
},
'& .react-select__menu': {
maxWidth: 440,
},
'& p': {
lineHeight: '1.43rem',
margin: 0,
maxWidth: '100%',
},
},
root: {
'& .mlMain': {
flexBasis: '100%',
maxWidth: '100%',
[theme.breakpoints.up('lg')]: {
flexBasis: '78.8%',
maxWidth: '78.8%',
},
},
'& .mlSidebar': {
flexBasis: '100%',
maxWidth: '100%',
position: 'static',
[theme.breakpoints.up('lg')]: {
flexBasis: '21.2%',
maxWidth: '21.2%',
position: 'sticky',
},
width: '100%',
},
},
sidebar: {
background: 'none',
marginTop: '0px !important',
paddingTop: '0px !important',
[theme.breakpoints.down('lg')]: {
background: theme.color.white,
marginTop: `${theme.spacing(3)} !important`,
padding: `${theme.spacing(3)} !important`,
},
[theme.breakpoints.down('md')]: {
padding: `${theme.spacing()} !important`,
},
},
}));
183 changes: 56 additions & 127 deletions packages/manager/src/features/Kubernetes/CreateCluster/CreateCluster.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import {
KubeNodePoolResponse,
} from '@linode/api-v4/lib/kubernetes';
import { APIError } from '@linode/api-v4/lib/types';
import { Divider } from '@mui/material';
import Grid from '@mui/material/Unstable_Grid2';
import { Theme } from '@mui/material/styles';
import { makeStyles } from '@mui/styles';
import { pick, remove, update } from 'ramda';
import * as React from 'react';
import { useHistory } from 'react-router-dom';
Expand Down Expand Up @@ -45,80 +44,13 @@ import { filterCurrentTypes } from 'src/utilities/filterCurrentLinodeTypes';
import { plansNoticesUtils } from 'src/utilities/planNotices';
import { LKE_HA_PRICE } from 'src/utilities/pricing/constants';
import { getDCSpecificPrice } from 'src/utilities/pricing/dynamicPricing';
import { doesRegionHaveUniquePricing } from 'src/utilities/pricing/linodes';
import { scrollErrorIntoView } from 'src/utilities/scrollErrorIntoView';

import KubeCheckoutBar from '../KubeCheckoutBar';
import { useStyles } from './CreateCluster.styles';
import { HAControlPlane } from './HAControlPlane';
import { NodePoolPanel } from './NodePoolPanel';
import { doesRegionHaveUniquePricing } from 'src/utilities/pricing/linodes';

const useStyles = makeStyles((theme: Theme) => ({
inner: {
'& > div': {
marginBottom: theme.spacing(2),
},
'& label': {
color: theme.color.headline,
fontWeight: 600,
letterSpacing: '0.25px',
lineHeight: '1.33rem',
margin: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those were the offenders. Those styles have been there since day one this feature was created.

A good reminder to be careful with nested global selectors.

},
},
inputWidth: {
'& .react-select__menu': {
maxWidth: 440,
},
maxWidth: 440,
},
regionSubtitle: {
'& .MuiInput-root': {
maxWidth: 440,
},
'& .react-select__menu': {
maxWidth: 440,
},
'& p': {
lineHeight: '1.43rem',
margin: 0,
maxWidth: '100%',
},
},
root: {
'& .mlMain': {
flexBasis: '100%',
maxWidth: '100%',
[theme.breakpoints.up('lg')]: {
flexBasis: '78.8%',
maxWidth: '78.8%',
},
},
'& .mlSidebar': {
flexBasis: '100%',
maxWidth: '100%',
position: 'static',
[theme.breakpoints.up('lg')]: {
flexBasis: '21.2%',
maxWidth: '21.2%',
position: 'sticky',
},
width: '100%',
},
},
sidebar: {
background: 'none',
marginTop: '0px !important',
paddingTop: '0px !important',
[theme.breakpoints.down('lg')]: {
background: theme.color.white,
marginTop: `${theme.spacing(3)} !important`,
padding: `${theme.spacing(3)} !important`,
},
[theme.breakpoints.down('md')]: {
padding: `${theme.spacing()} !important`,
},
},
}));

export const CreateCluster = () => {
const classes = useStyles();
Expand Down Expand Up @@ -289,61 +221,60 @@ export const CreateCluster = () => {
<Grid className={`mlMain py0`}>
{errorMap.none && <Notice text={errorMap.none} variant="error" />}
<Paper data-qa-label-header>
<div className={classes.inner}>
<TextField
onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
updateLabel(e.target.value)
}
className={classes.inputWidth}
data-qa-label-input
errorText={errorMap.label}
label="Cluster Label"
value={label || ''}
/>
<RegionSelect
handleSelection={(regionID: string) =>
setSelectedRegionID(regionID)
}
textFieldProps={{
helperText: <RegionHelperText mb={2} />,
helperTextPosition: 'top',
}}
className={classes.regionSubtitle}
errorText={errorMap.region}
regions={filteredRegions}
selectedID={selectedID}
/>
{showPricingNotice && (
<DynamicPriceNotice
region={selectedRegionID}
spacingBottom={16}
<TextField
onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
updateLabel(e.target.value)
}
className={classes.inputWidth}
data-qa-label-input
errorText={errorMap.label}
label="Cluster Label"
value={label || ''}
/>
<Divider sx={{ marginTop: 4 }} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question, I've noticed in other places of the code when we pass a styling value via sx prop like in this fashion we at time use included the css unit like this sx= {{ marginTop: '20px' }}. If I append the css unit to this line it changes the UI.

Have we established a preferred method to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. SX is nice because the spacing it tied to our design system. (aka theme.spacing)

on the other hand, sx= {{ marginTop: '20px' }} is not great cause you are assigning a hard coded value that would remain immutable to a design change (say you want to modify our spacing at the theme level, or to a "compact theme" etc)

Whenever possible, us sx and theme values, or theme.spacing(x) in styled components and makeStyles

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I usually pass the theme prop to sx to take utilize the spacing values like this:
sx={(theme) => ({ marginTop: theme.spacing(2) })}

However, in this specific case the theme is not available, so we can just use number values. Thanks for the explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, theme is available if I want to pass it, it's more that I don't need it in this case since I am only worried about this particular CSS property. doing sx={(theme) => ({ marginTop: theme.spacing(x) })} would achieve the same thing and be just as right.

<RegionSelect
handleSelection={(regionID: string) =>
setSelectedRegionID(regionID)
}
textFieldProps={{
helperText: <RegionHelperText mb={2} />,
helperTextPosition: 'top',
}}
className={classes.regionSubtitle}
errorText={errorMap.region}
regions={filteredRegions}
selectedID={selectedID}
/>
{showPricingNotice && (
<DynamicPriceNotice region={selectedRegionID} spacingBottom={16} />
)}
<Divider sx={{ marginTop: 4 }} />
<Select
onChange={(selected: Item<string>) => {
setVersion(selected);
}}
className={classes.inputWidth}
errorText={errorMap.k8s_version}
isClearable={false}
label="Kubernetes Version"
options={versions}
placeholder={' '}
value={version || null}
/>
<Divider sx={{ marginTop: 4 }} />
{showHighAvailability ? (
<Box data-testid="ha-control-plane">
<HAControlPlane
highAvailabilityPrice={
flags.dcSpecificPricing
? getHighAvailabilityPrice(selectedID)
: LKE_HA_PRICE
}
setHighAvailability={setHighAvailability}
/>
)}
<Select
onChange={(selected: Item<string>) => {
setVersion(selected);
}}
className={classes.inputWidth}
errorText={errorMap.k8s_version}
isClearable={false}
label="Kubernetes Version"
options={versions}
placeholder={' '}
value={version || null}
/>
{showHighAvailability ? (
<Box data-testid="ha-control-plane">
<HAControlPlane
highAvailabilityPrice={
flags.dcSpecificPricing
? getHighAvailabilityPrice(selectedID)
: LKE_HA_PRICE
}
setHighAvailability={setHighAvailability}
/>
</Box>
) : null}
</div>
</Box>
) : null}
<Divider sx={{ marginBottom: 4 }} />
<NodePoolPanel
typesError={
typesError
Expand Down Expand Up @@ -402,5 +333,3 @@ export const CreateCluster = () => {
</Grid>
);
};

export default CreateCluster;
1 change: 0 additions & 1 deletion packages/manager/src/features/Kubernetes/CreateCluster/index.tsx
View file
Open in desktop

This file was deleted.

6 changes: 5 additions & 1 deletion packages/manager/src/features/Kubernetes/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import { SuspenseLoader } from 'src/components/SuspenseLoader';
const KubernetesLanding = React.lazy(
() => import('./KubernetesLanding/KubernetesLanding')
);
const ClusterCreate = React.lazy(() => import('./CreateCluster'));
const ClusterCreate = React.lazy(() =>
import('./CreateCluster/CreateCluster').then((module) => ({
default: module.CreateCluster,
}))
);
const ClusterDetail = React.lazy(() => import('./KubernetesClusterDetail'));

const Kubernetes: React.FC = () => {
Expand Down