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

feat: parameters inputs #755

Merged
merged 5 commits into from
Dec 22, 2020
Merged

feat: parameters inputs #755

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
69f950c
feat: parameter validations
mallachari Dec 11, 2020
b199312
chore: review suggestions
mallachari Dec 14, 2020
78477c5
chore: cleanup and separete Parameter component
mallachari Dec 22, 2020
524984c
Merge branch 'v7' into feat/parameters-inputs
mallachari Dec 22, 2020
ec8e5a3
test: update tests
mallachari Dec 22, 2020
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
29 changes: 29 additions & 0 deletions packages/elements/src/__fixtures__/operations/put-todos.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,35 @@ export const httpOperation: IHttpOperation = {
name: 'account-id',
style: HttpParamStyles.Simple,
required: true,
examples: [
{
value: 'example id',
key: 'example',
},
],
},
{
schema: {
type: 'string',
description: 'Your Stoplight account id',
},
name: 'message-id',
style: HttpParamStyles.Simple,
required: true,
examples: [
{
value: 'example value',
key: 'example 1',
},
{
value: 'another example',
key: 'example 2',
},
{
value: 'something else',
key: 'example 3',
},
],
},
],
path: [
Expand Down
6 changes: 4 additions & 2 deletions packages/elements/src/components/TryIt/BasicSend.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as React from 'react';

import { HttpCodeDescriptions } from '../../constants';
import { getHttpCodeColor } from '../../utils/http';
import { OperationParameters } from './OperationParameters';
import { initialParameterValues, OperationParameters } from './OperationParameters';

export interface BasicSendProps {
httpOperation: IHttpOperation;
Expand All @@ -33,7 +33,9 @@ export const BasicSend: React.FC<BasicSendProps> = ({ httpOperation }) => {
};
const allParameters = Object.values(operationParameters).flat();

const [parameterValues, setParameterValues] = React.useState<Dictionary<string, string>>({});
const [parameterValues, setParameterValues] = React.useState<Dictionary<string, string>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just noticing, that we are defining a lot of stuff in BasicSend, but then it is only used in OperationParameters really, with an exception of building the actual fetch request.

Since operation parameters are actually a responsibility of... well... OperationParameters component, maybe all this logic - initialParameterValues, operationParameters etc. should be placed in OperationParameters?

This is a soft proposition though - not really sure if the code would become actually cleaner in reality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, we do. I guess the idea was to keep state in main component as there might be more components using that state. I'm not sure whether we should move it. We will eventually switch to a state library so then might be good time to clear it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a hard one. This is what you end up with when you do the playbook React "lift the state up" exercise, and I'm not sure it's a bad thing. The more you lift the state up the more OperationParameters (the descendant) becomes a fully controlled presentational component. So while on one hand I think you are right on the other this is better separation of state logic vs visualization.

I think it's fine to leave it as is, at least for now. Also it isn't something that I would bring in a state management lib for, I think what vanilla React offers here is perfectly good enough and doesn't justify the complexity but that's another discussion.

initialParameterValues(operationParameters),
);
marcelltoth marked this conversation as resolved.
Show resolved Hide resolved

if (!server) return null;

Expand Down
129 changes: 108 additions & 21 deletions packages/elements/src/components/TryIt/OperationParameters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import { Flex, Input, Panel, Text } from '@stoplight/mosaic';
import { Dictionary, IHttpHeaderParam, IHttpPathParam, IHttpQueryParam } from '@stoplight/types';
import { sortBy } from 'lodash';
import { Flex, Input, Panel, Select, Text } from '@stoplight/mosaic';
import {
Dictionary,
IHttpHeaderParam,
IHttpParam,
IHttpPathParam,
IHttpQueryParam,
INodeExample,
INodeExternalExample,
} from '@stoplight/types';
import { map, sortBy } from 'lodash';
import * as React from 'react';

interface OperationParameters {
export interface OperationParameters {
path?: IHttpPathParam[];
query?: IHttpQueryParam[];
headers?: IHttpHeaderParam[];
Expand All @@ -15,41 +23,120 @@ interface OperationParametersProps {
onChangeValues: (newValues: Dictionary<string, string>) => void;
}

const booleanOptions = [
{ label: 'Not Set', value: '' },
{ label: 'False', value: 'false' },
{ label: 'True', value: 'true' },
];

const selectExampleOption = { value: '', label: 'Pick an example' };

export const OperationParameters: React.FC<OperationParametersProps> = ({
operationParameters,
values,
onChangeValues,
}) => {
const pathParameters = sortBy(operationParameters.path ?? [], ['name']);
const queryParameters = sortBy(operationParameters.query ?? [], ['name']);
const headerParameters = sortBy(operationParameters.headers ?? [], ['name']);
const parameters = [...pathParameters, ...queryParameters, ...headerParameters];
const parameters = flattenParameters(operationParameters);

const onChange = (parameter: IHttpParam) => (
e: React.FormEvent<HTMLSelectElement> | React.ChangeEvent<HTMLInputElement>,
) => {
const newValue = e.currentTarget.value;
onChangeValues({ ...values, [parameter.name]: newValue });
};

return (
<Panel id="collapse-open" defaultIsOpen>
<Panel.Titlebar>Parameters</Panel.Titlebar>
<Panel.Content className="sl-overflow-y-auto OperationParametersContent">
{parameters.map(parameter => {
const parameterValueOptions = parameterOptions(parameter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could clean up this map by sepearating those children into a separate component that simply accepts parameter prop (and some other stuff if necessary)

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 point - I moved it.

const examples = exampleOptions(parameter);
const selectedExample = examples?.find(e => e.value === values[parameter.name]) ?? selectExampleOption;
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be separated into getExampleForParameter util or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's worth it as parameterValueOptions and examples are used here anyway so that would be just replacing that one line. Is it?

return (
<Flex key={parameter.name} alignItems="center">
<Flex align="center" key={parameter.name}>
<Input appearance="minimal" readOnly value={parameter.name} />
<Text mx={3}>:</Text>
<Input
appearance="minimal"
flexGrow
placeholder={parameter.schema?.type as string}
type={parameter.schema?.type as string}
required
value={values[parameter.name] ?? ''}
onChange={e => {
const newValue = e.currentTarget.value;
onChangeValues({ ...values, [parameter.name]: newValue });
}}
/>
{parameterValueOptions ? (
<Select
flexGrow
options={parameterValueOptions}
value={values[parameter.name]}
onChange={onChange(parameter)}
/>
) : (
<Flex flexGrow>
<Input
style={{ paddingLeft: 15 }}
appearance="minimal"
flexGrow
placeholder={(parameter.schema?.default ?? parameter.schema?.type) as string}
type={parameter.schema?.type as string}
Copy link
Contributor

Choose a reason for hiding this comment

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

types say that this might be an array as well.

How do we know it will be a string for sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not be a string, but this was wrong anyway. That's not a change I did in this PR, but input type shouldn't be set to parameter type. It should be either text or number in our case, definitely not string, array or boolean

required
value={values[parameter.name] ?? ''}
onChange={onChange(parameter)}
/>
{examples && (
<Select flexGrow value={selectedExample.value} options={examples} onChange={onChange(parameter)} />
)}
</Flex>
)}
</Flex>
);
})}
</Panel.Content>
</Panel>
);
};

function flattenParameters(parameters: OperationParameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In Haskell etc. it is a common practice to put "util" functions alongside the defined interface.

So in this case, the interface OperationParameters would land in it's own file and all associated functions would be placed in the same file as well.

Parhaps worth considering here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. At that point those will become exports which is more commitment than some random internal functions here alongside the component. I see your point but I'd say it isn't necessarily worth here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an idea that I haven't heard before. We could add it to consideration along others code structure ideas.

const pathParameters = sortBy(parameters.path ?? [], ['name']);
const queryParameters = sortBy(parameters.query ?? [], ['name']);
const headerParameters = sortBy(parameters.headers ?? [], ['name']);
return [...pathParameters, ...queryParameters, ...headerParameters];
}

export function initialParameterValues(operationParameters: OperationParameters) {
const parameters = flattenParameters(operationParameters);

const enums = Object.fromEntries(
parameters
.map(p => [p.name, p.schema?.enum ?? []] as const)
.filter(([, enums]) => enums.length > 0)
.map(([name, enums]) => [name, String(enums[0])]),
);

const examples = Object.fromEntries(
parameters
.map(p => [p.name, p.examples ?? []] as const)
.filter(([, examples]) => examples.length > 0)
.map(([name, examples]) => [name, exampleValue(examples[0])]),
);

return {
// order matters - enums should be override examples
...examples,
...enums,
};
}

function parameterOptions(parameter: IHttpParam) {
return parameter.schema?.type === 'boolean'
? booleanOptions
: parameter.schema?.enum !== undefined
? map(parameter.schema.enum, v => (Number.isNaN(Number(v)) ? String(v) : Number(v)))
: null;
}

function exampleOptions(parameter: IHttpParam) {
return parameter.examples?.length && parameter.examples.length > 1
? [
selectExampleOption,
...parameter.examples.map(example => ({ label: example.key, value: exampleValue(example) })),
]
: null;
}

function exampleValue(example: INodeExample | INodeExternalExample) {
return 'value' in example ? example.value : example.externalValue;
}
22 changes: 22 additions & 0 deletions packages/elements/src/components/TryIt/__tests__/parameters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { httpOperation } from '../../../__fixtures__/operations/put-todos';
import { initialParameterValues } from '../OperationParameters';

describe('Parameters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I believe that - according to what we agreed on - this shouldn't really be tested.

It should be tested if the component itself displays proper contents on initial render? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the test of that function, but I agree, there should also be a component test

it('should fill initial parameters', () => {
const operationParameters = {
path: httpOperation.request?.path,
query: httpOperation.request?.query,
headers: httpOperation.request?.headers,
};

const parameters = initialParameterValues(operationParameters);

expect(parameters).toMatchObject({
limit: '0',
type: 'something',
value: '0',
'account-id': 'example id',
'message-id': 'example value',
});
});
});