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

Bug 1829385: fix alignment issues with MultiColumnField in Traffic Splitting modal #5229

Merged
merged 1 commit into from
May 7, 2020
Merged

Bug 1829385: fix alignment issues with MultiColumnField in Traffic Splitting modal #5229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion frontend/packages/console-shared/src/components/formik-fields/field-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ValidatedOptions, TextInputTypes } from '@patternfly/react-core';
import { ValidatedOptions, TextInputTypes, gridItemSpanValueShape } from '@patternfly/react-core';
import { K8sResourceKind } from '@console/internal/module/k8s';

export interface FieldProps {
Expand Down Expand Up @@ -79,6 +79,7 @@ export interface MultiColumnFieldProps extends FieldProps {
emptyMessage?: string;
headers: string[];
children: React.ReactNode;
spans?: gridItemSpanValueShape[];
}

export interface YAMLEditorFieldProps extends FieldProps {
Expand Down
6 changes: 2 additions & 4 deletions ...ages/console-shared/src/components/formik-fields/multi-column-field/MultiColumnField.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
.odc-multi-column-field {
&__row {
display: flex;
flex-direction: row;
position: relative;
margin-right: var(--pf-global--spacer--lg);
margin-bottom: var(--pf-global--spacer--lg);
max-height: var(--pf-global--spacer--xl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw that we have a max height per row. unsure why that is. @rohitkrai03 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @rohitkrai03 ^

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't exactly remember as to why I added this but I think it was something to do with the UX designs at the moment and rows being bigger than the designs.

}

&__col {
flex: 1 0 0;
margin-right: var(--pf-global--spacer--md);
}

&__col--button {
cursor: pointer;
line-height: var(--pf-global--spacer--xl);
position: absolute;
right: -30px;
top: 50%;
transform: translateY(-50%);
}
Comment on lines +17 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, wonder if we could make a slight change here.

cursor: pointer; impacts disabled buttons... Moving it to here would make it only cursor when you can click the button:

    button:not(:disabled) {
      cursor: pointer;
    }

&__empty-message {
margin-bottom: var(--pf-global--spacer--sm);
Expand Down
11 changes: 9 additions & 2 deletions ...kages/console-shared/src/components/formik-fields/multi-column-field/MultiColumnField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import * as React from 'react';
import * as _ from 'lodash';
import { FieldArray, useFormikContext, FormikValues } from 'formik';
import { FormGroup } from '@patternfly/react-core';
import { FormGroup, gridItemSpanValueShape } from '@patternfly/react-core';
import { SecondaryStatus, useFormikValidationFix } from '@console/shared';
import { MultiColumnFieldProps } from '../field-types';
import MultiColumnFieldHeader from './MultiColumnFieldHeader';
import MultiColumnFieldRow from './MultiColumnFieldRow';
import MultiColumnFieldFooter from './MultiColumnFieldFooter';
import { getSpans } from './multicolumn-field-utils';
import './MultiColumnField.scss';

const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
Expand All @@ -23,9 +24,14 @@ const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
disableDeleteRow,
disableAddRow,
toolTip,
spans,
}) => {
const { values } = useFormikContext<FormikValues>();
const fieldValue = _.get(values, name, []);
const totalFieldCount: gridItemSpanValueShape = React.Children.count(
children,
) as gridItemSpanValueShape;
Comment on lines +31 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

Typing and casting should not be needed... did TypeScript yell at you?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, after changing the type of totalFieldCount to gridItemSpanValueShape from number in the utils.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, i caused this because I didn't follow the code while making the suggestion for the input param type.
Don't care that much which approach you take so long as the getSpans function supports a number > 12.

const fieldSpans = spans || getSpans(totalFieldCount);
useFormikValidationFix(fieldValue);
return (
<FieldArray
Expand All @@ -45,7 +51,7 @@ const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
</div>
)
) : (
<MultiColumnFieldHeader headers={headers} />
<MultiColumnFieldHeader headers={headers} spans={fieldSpans} />
)}
{fieldValue.length > 0 &&
fieldValue.map((value, index) => (
Expand All @@ -57,6 +63,7 @@ const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
onDelete={() => remove(index)}
isReadOnly={isReadOnly}
disableDeleteRow={disableDeleteRow}
spans={fieldSpans}
>
{children}
</MultiColumnFieldRow>
Expand Down
16 changes: 10 additions & 6 deletions ...console-shared/src/components/formik-fields/multi-column-field/MultiColumnFieldHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
import * as React from 'react';
import { Grid, GridItem, gridItemSpanValueShape } from '@patternfly/react-core';
import './MultiColumnField.scss';

export interface MultiColumnFieldHeaderProps {
headers: string[];
spans: gridItemSpanValueShape[];
}

const MultiColumnFieldHeader: React.FC<MultiColumnFieldHeaderProps> = ({ headers }) => (
const MultiColumnFieldHeader: React.FC<MultiColumnFieldHeaderProps> = ({ headers, spans }) => (
<div className="odc-multi-column-field__row">
{headers.map((header) => (
<div className="odc-multi-column-field__col" key={header}>
{header}
</div>
))}
<Grid className="odc-multi-column-field__row">
{headers.map((header, i) => (
<GridItem span={spans[i]} key={header}>
<div className="odc-multi-column-field__col">{header}</div>
</GridItem>
))}
</Grid>
<div className="odc-multi-column-field__col--button" />
</div>
);
Expand Down
32 changes: 22 additions & 10 deletions ...es/console-shared/src/components/formik-fields/multi-column-field/MultiColumnFieldRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import * as React from 'react';
import { MinusCircleIcon } from '@patternfly/react-icons';
import { Tooltip, Button, ButtonVariant, ButtonType } from '@patternfly/react-core';
import {
Tooltip,
Button,
ButtonVariant,
ButtonType,
GridItem,
Grid,
gridItemSpanValueShape,
} from '@patternfly/react-core';
import './MultiColumnField.scss';

export interface MultiColumnFieldRowProps {
Expand All @@ -11,6 +19,7 @@ export interface MultiColumnFieldRowProps {
onDelete: () => void;
isReadOnly?: boolean;
disableDeleteRow?: boolean;
spans: gridItemSpanValueShape[];
}

const minusCircleIcon = (onDelete: () => void, disableDeleteRow?: boolean, toolTip?: string) => {
Expand Down Expand Up @@ -51,17 +60,20 @@ const MultiColumnFieldRow: React.FC<MultiColumnFieldRowProps> = ({
children,
isReadOnly,
disableDeleteRow,
spans,
}) => (
<div className="odc-multi-column-field__row">
{React.Children.map(children, (child: React.ReactElement) => {
const fieldName = `${name}.${rowIndex}.${child.props.name}`;
const newProps = { ...child.props, name: fieldName };
return (
<div key={fieldName} className="odc-multi-column-field__col">
{React.cloneElement(child, newProps)}
</div>
);
})}
<Grid>
{React.Children.map(children, (child: React.ReactElement, i) => {
const fieldName = `${name}.${rowIndex}.${child.props.name}`;
const newProps = { ...child.props, name: fieldName };
return (
<GridItem span={spans[i]} key={fieldName}>
<div className="odc-multi-column-field__col">{React.cloneElement(child, newProps)}</div>
</GridItem>
);
})}
</Grid>
{!isReadOnly && renderMinusCircleIcon(onDelete, toolTip, disableDeleteRow)}
</div>
);
Expand Down
28 changes: 28 additions & 0 deletions ...src/components/formik-fields/multi-column-field/__tests__/multicolumn-field-utils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { getSpans } from '../multicolumn-field-utils';

describe('Multicolumn field utils', () => {
it('should return single spans value', () => {
const spans = getSpans(1);
expect(spans).toEqual([12]);
});
it('should return 12 spans equally sized', () => {
const spans = getSpans(12);
expect(spans).toEqual([1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]);
});
it('should return spans [3, 3, 2, 2, 2]', () => {
const spans = getSpans(5);
expect(spans).toEqual([3, 3, 2, 2, 2]);
});
it('should return spans [2, 2, 2, 2, 2, 1, 1]', () => {
const spans = getSpans(7);
expect(spans).toEqual([2, 2, 2, 2, 2, 1, 1]);
});
it('should return spans [2, 2, 2, 1, 1, 1, 1, 1, 1]', () => {
const spans = getSpans(9);
expect(spans).toEqual([2, 2, 2, 1, 1, 1, 1, 1, 1]);
});
it('should return spans [4, 4, 4]', () => {
const spans = getSpans(3);
expect(spans).toEqual([4, 4, 4]);
});
});
17 changes: 17 additions & 0 deletions ...console-shared/src/components/formik-fields/multi-column-field/multicolumn-field-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import * as _ from 'lodash';
import { gridItemSpanValueShape } from '@patternfly/react-core';

export const getSpans = (totalFieldCount: gridItemSpanValueShape): gridItemSpanValueShape[] => {
const spans: gridItemSpanValueShape[] = _.fill(
Array(totalFieldCount),
Math.trunc(12 / totalFieldCount),
) as gridItemSpanValueShape[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, is the type & cast needed by TypeScript?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was getting the type error because after adding return type gridItemSpanValueShape[] to this utils.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right... because _.fill has probably a very generic type (if any). Well, it's a nit at this point, but you don't have to type the variable if you're casting. It's always that type.


let remainder = 12 % totalFieldCount;

while (remainder--) {
spans[remainder]++;
}

return spans;
};
1 change: 1 addition & 0 deletions frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const TrafficSplittingModal: React.FC<Props> = ({
headers={['Split', 'Tag', 'Revision']}
emptyValues={{ percent: '', tag: '', revisionName: '' }}
disableDeleteRow={values.trafficSplitting.length === 1}
spans={[2, 3, 7]}
>
<InputField
name="percent"
Expand Down