feat: parameters inputs #755

mallachari · 2020-12-11T08:58:21Z

Fixes #645

Adds parameter validations:

enums as dropdown - first element as default (we could add Not Set as with booleans)
booleans as dropdown - Not Set (empty string) as default
examples - first item as populated value, if more than one show both - text field and dropdown
default as placeholder

Initializes parameters with values from enums and examples.

philsturgeon · 2020-12-11T11:24:30Z

This is looking pretty good! One slight deviation from the plan is the example dropdown.

The idea is that a string with an example is still just a string, there's simply a selector that allows you to pick an example to update that string value. Same for numbers, they're a input[type=number] which also allows you to select the example.

It's not an input next to a selector, its an input with a selector.

mallachari · 2020-12-11T13:41:04Z

This is looking pretty good! One slight deviation from the plan is the example dropdown.
The idea is that a string with an example is still just a string, there's simply a selector that allows you to pick an example to update that string value. Same for numbers, they're a input[type=number] which also allows you to select the example.

It's not an input next to a selector, its an input with a selector.

Right, I get it. Thing is that having regular input with selector is tricky if we want to stick to current mosaic components.
This issue is marked as blocked by https://github.com/stoplightio/mosaic/issues/44 which might change that (not sure as it's empty), but for now we might need to stick with what we have. Unless I'm missing some feature.

There's a solution with list prop and dataset, but that doesn't look good.

@marcelltoth what do you think? If there's no way to handle it for now we could stick to initial approach and single input with first example populated.

philsturgeon · 2020-12-11T18:34:01Z

We can split work until later if it’s blocked but please make a new issue and mention it in the PR and team leader agreed so we can get it added to the backlog. We can leave it now it is until mosaic is ready to help, or we can make mosaic support it, whichever you folks decide, so long as there’s an issue to finish it later.

…

-- Phil Sturgeon DevRel @ Stoplight.io

On Dec 11, 2020, at 13:41, Jakub Jankowski ***@***.***> wrote: This is looking pretty good! One slight deviation from the plan is the example dropdown. The idea is that a string with an example is still just a string, there's simply a selector that allows you to pick an example to update that string value. Same for numbers, they're a input[type=number] which also allows you to select the example. It's not an input next to a selector, its an input with a selector. Right, I get it. Thing is that having regular input with selector is tricky if we want to stick to current mosaic components. This issue is marked as blocked by stoplightio/mosaic#44 which might change that (not sure as it's empty), but for now we might need to stick with what we have. Unless I'm missing some feature. There's a solution with list prop and dataset, but that doesn't look good. @marcelltoth what do you think? If there's no way to handle it for now we could stick to initial approach and single input with first example populated. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

netlify · 2020-12-11T19:03:18Z

✔️ Deploy preview for stoplight-elements ready!

🔨 Explore the source changes: ec8e5a3

🔍 Inspect the deploy logs: https://app.netlify.com/sites/stoplight-elements/deploys/5fe1ca5206413600094bf066

😎 Browse the preview: https://deploy-preview-755--stoplight-elements.netlify.app

mpodlasin · 2020-12-14T10:44:46Z

packages/elements/src/components/TryIt/__tests__/parameters.test.ts

+import { httpOperation } from '../../../__fixtures__/operations/put-todos';
+import { initialParameterValues } from '../OperationParameters';
+
+describe('Parameters', () => {


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? 🤔

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

marcelltoth

Enums work perfectly.

The workaround for Examples is perfect, too, IMO, just create a followup issue as @philsturgeon requested.

I left some comments in the code. It's a first round so I might have skipped something, will check again when these are resolved or dismissed.

packages/elements/src/components/TryIt/OperationParameters.tsx

marcelltoth · 2020-12-14T10:21:40Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+  };
+}
+
+function parameterOptions(parameter: IHttpPathParam | IHttpQueryParam) {


As it stands, this function has a weird return type:

It would be nice to normalize it to one or the other.

You could change the nested ternaries into ifs, it might be longer but easier to read.

The parameter can be IHttpParam which is the common denominator between the two inputs.

Right, IHttpParam does the job

marcelltoth · 2020-12-14T10:25:11Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+function exampleOptions(parameter: IHttpPathParam | IHttpQueryParam) {
+  return parameter.examples?.length && parameter.examples.length > 1
+    ? parameter.examples.map(example => ({ label: example.key, value: exampleValue(example) }))
+    : null;
+}


This one has a nice return type for example 👍

marcelltoth · 2020-12-14T10:28:09Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+  return 'value' in example
+    ? (example as INodeExample).value
+    : 'externalValue' in example
+    ? (example as INodeExternalExample).externalValue
+    : '';


This simplifies to

return 'value' in example ? example.value : example.externalValue;

Fortunately in is now a type-guard. (Which is a bit of a lie in TypeScript as there are no final-types but it's handy). If the types don't lie the third case - your '' - should never appear.

This is great!

marcelltoth · 2020-12-14T10:51:49Z

packages/elements/src/components/TryIt/OperationParameters.tsx

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


This useCallback doesn't give you anything unfortunately. The function that will be memoized is the outer one that isn't used as a prop directly. I think you can simply lose the useCallback and be fine, but if you really want to memoize it the only way I know of is something like:

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

If I'm not mistaken this should now have the type of Array<(e: React.FormEvent<HTMLSelectElement> | React.ChangeEvent<HTMLInputElement>) => void> then you can use it as onChange={onChangeFunctions[parameter]}.

But probably not worth the effort TBH, unless we actually measure some performance bottleneck. (Which I don't expect as there shouldn't be much more than 10-20 parameters IMO)

True, not worth it in this case.

marcelltoth · 2020-12-14T11:08:11Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+                    value={values[parameter.name] ?? ''}
+                    onChange={onChange(parameter)}
+                  />
+                  {examples && <Select flexGrow options={examples} onChange={onChange(parameter)} />}


This input is partially-controlled which is usually not advised. If we provide onChange we should provide value as well. It will improve UX as well because we could provide value like:

//somewhere up as a constant const selectExampleOption = {value: '', label: "Pick an example"}; // in the map const examples = // this should include selectExampleOption const selectedExample = examples.find(e => e.value === values[parameter.name]) ?? selectExampleOption; <Select value={selectedExample.value} />

This will make the example selector react to manual changes as well: in case the user types something it'll reset to "Pick an example"

marcelltoth · 2020-12-14T12:27:55Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+  const examples = parameters
+    .filter(p => Array.isArray(p.examples))
+    .reduce((params, p) => {
+      if (p.examples?.length) {
+        return {
+          ...params,
+          [p.name]: exampleValue(p.examples[0]),
+        };
+      } else {
+        return { ...params };
+      }
+    }, {});


These reduces can be written in a more declarative way using Object.fromEntries which is usually nicer IMO. Your call if you like it or not, also applies to the one above. This example:

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])]), );

I remember you told me about that earlier. I also prefer your way when I look at it, I'm just too used to reduce ;)

packages/elements/src/components/TryIt/BasicSend.tsx

mpodlasin · 2020-12-21T10:38:55Z

packages/elements/src/components/TryIt/BasicSend.tsx

@@ -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>>(


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.

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.

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.

mpodlasin · 2020-12-21T10:47:49Z

packages/elements/src/components/TryIt/OperationParameters.tsx


  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);


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)

Good point - I moved it.

mpodlasin · 2020-12-21T10:49:48Z

packages/elements/src/components/TryIt/OperationParameters.tsx


  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);
+          const examples = exampleOptions(parameter);
+          const selectedExample = examples?.find(e => e.value === values[parameter.name]) ?? selectExampleOption;


this might be separated into getExampleForParameter util or something.

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?

mpodlasin · 2020-12-21T10:52:29Z

packages/elements/src/components/TryIt/OperationParameters.tsx

+                    appearance="minimal"
+                    flexGrow
+                    placeholder={(parameter.schema?.default ?? parameter.schema?.type) as string}
+                    type={parameter.schema?.type as string}


types say that this might be an array as well.

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

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

mpodlasin · 2020-12-21T10:53:44Z

packages/elements/src/components/TryIt/OperationParameters.tsx

            </Flex>
          );
        })}
      </Panel.Content>
    </Panel>
  );
 };
+
+function flattenParameters(parameters: OperationParameters) {


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?

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.

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

marcelltoth

Lovely! 🚢 IT!

marcelltoth · 2020-12-22T10:51:03Z

packages/elements/src/components/TryIt/__tests__/BasicSend.test.tsx

+      const completedField = screen.getByLabelText('completed');
+      expect(completedField).toHaveValue('');


I love the getByLabelText solution! 😍

philsturgeon

Amazing!

mallachari requested a review from philsturgeon as a code owner December 11, 2020 08:58

mallachari requested a review from a team December 11, 2020 08:58

mallachari self-assigned this Dec 11, 2020

Base automatically changed from feat/try-it-set-parameters to v7 December 11, 2020 17:23

feat: parameter validations

69f950c

mallachari force-pushed the feat/parameters-inputs branch from 96cd82e to 69f950c Compare December 11, 2020 18:59

mpodlasin reviewed Dec 14, 2020

View reviewed changes

marcelltoth reviewed Dec 14, 2020

View reviewed changes

chore: review suggestions

b199312

mallachari requested a review from marcelltoth December 14, 2020 23:45

mpodlasin reviewed Dec 21, 2020

View reviewed changes

marcelltoth mentioned this pull request Dec 21, 2020

Try It: Allow selecting parameter enums & examples #645

Closed

Jakub Jankowski added 3 commits December 22, 2020 09:51

chore: cleanup and separete Parameter component

78477c5

Merge branch 'v7' into feat/parameters-inputs

524984c

test: update tests

ec8e5a3

mallachari requested a review from mpodlasin December 22, 2020 10:41

marcelltoth approved these changes Dec 22, 2020

View reviewed changes

philsturgeon approved these changes Dec 22, 2020

View reviewed changes

mallachari merged commit fec2d79 into v7 Dec 22, 2020

mallachari deleted the feat/parameters-inputs branch December 22, 2020 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: parameters inputs #755

feat: parameters inputs #755

mallachari commented Dec 11, 2020 •

edited by marcelltoth

Loading

philsturgeon commented Dec 11, 2020

mallachari commented Dec 11, 2020

philsturgeon commented Dec 11, 2020 via email

netlify bot commented Dec 11, 2020 •

edited

Loading

mpodlasin Dec 14, 2020

mallachari Dec 14, 2020

marcelltoth left a comment

marcelltoth Dec 14, 2020

marcelltoth Dec 14, 2020

mallachari Dec 14, 2020

marcelltoth Dec 14, 2020

marcelltoth Dec 14, 2020

mallachari Dec 14, 2020

marcelltoth Dec 14, 2020

mallachari Dec 14, 2020

marcelltoth Dec 14, 2020

marcelltoth Dec 14, 2020

mallachari Dec 14, 2020

mpodlasin Dec 21, 2020

mallachari Dec 22, 2020

marcelltoth Dec 22, 2020

mpodlasin Dec 21, 2020

mallachari Dec 22, 2020

mpodlasin Dec 21, 2020

mallachari Dec 22, 2020

mpodlasin Dec 21, 2020

mallachari Dec 22, 2020

mpodlasin Dec 21, 2020

marcelltoth Dec 22, 2020

mallachari Dec 22, 2020

marcelltoth left a comment •

edited

Loading

marcelltoth Dec 22, 2020

philsturgeon left a comment

		const completedField = screen.getByLabelText('completed');
		expect(completedField).toHaveValue('');

feat: parameters inputs #755

feat: parameters inputs #755

Conversation

mallachari commented Dec 11, 2020 • edited by marcelltoth Loading

philsturgeon commented Dec 11, 2020

mallachari commented Dec 11, 2020

philsturgeon commented Dec 11, 2020 via email

netlify bot commented Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelltoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelltoth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon left a comment

Choose a reason for hiding this comment

mallachari commented Dec 11, 2020 •

edited by marcelltoth

Loading

netlify bot commented Dec 11, 2020 •

edited

Loading

marcelltoth left a comment •

edited

Loading