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.

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

Form lib enhancements #78588

Merged
merged 27 commits into from
Oct 5, 2020
Merged

Form lib enhancements #78588

merged 27 commits into from
Oct 5, 2020

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Sep 28, 2020

This PR adds several enhancements to the form lib that I have made while working on the docs. Mainly refactors, TS types, comments.

  • Added Internal state interface to useForm

It is now possible to declare the internal state interface (I) when building the form.

interface MyForm {
  name: string;
};

interface MyFormInternal extends MyForm {
  showSettings: boolean;
}

const deserializer = (defaultValue: MyForm): MyFormInternal => {
  ...
};

const { form } = useForm<MyForm, MyFormInternal>({
  deserializer, // Typescript will fail if not returning the "MyFormInternal" interface
});
  • Improve Typing for FormSchema

When providing a schema to a typed form, it validates that fields name are valid. None of the fields are required though.

interface MyForm {
  name: string;
  lastName: string;
}

// Can only contain "name" or "lastName" fields
const schema: FormSchema<MyForm> = {
  name: {}
};
  • Refactor: renamed some vars
  • Simplify bits of code
  • Added field type to <UseMultiFields /> component
interface FieldsType {
  min: number;
  max: number;
}

// In JSX
<UseMultiFields<FieldsType> ... />
  • Removed handler on the form public API (getFieldDefaultValue())

Fixes #60602

@sebelga sebelga requested review from a team as code owners September 28, 2020 13:42
@sebelga sebelga requested a review from jloleysens September 28, 2020 13:42
@sebelga sebelga added release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0 labels Sep 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@sebelga sebelga mentioned this pull request Sep 28, 2020
3 tasks
@sebelga
Copy link
Contributor Author

sebelga commented Sep 29, 2020

@patrykkopycinski Could you have a look at those 2 files. There are typings issue now that the FormSchema<T> is correctly typed. We can have a look together if you want. Cheers! 👍

  • L78 x-pack/plugins/security_solution/public/cases/components/edit_connector/index.tsx
  • L192 x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

@patrykkopycinski
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Tested ingest processors pipeline locally, and did not spot any regressions!

Great work @sebelga ! I left non-blocking comments in the code.

readonly isSubmitting: boolean;
/** Flag that indicates if the form is valid. It can have three values: `true | false | undefined`. */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; I would phrase this as:

/** Flag that indicates if the form is valid. If `undefined` then form validation has not been checked yet. */

I think the TS boolean value is self-explanatory for the other states.

readonly id: string;
/**
* This handlers submits the form and returns its data and validity. If the form is not valid, the data will be `null`
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: handlers -> handler

@@ -18,7 +18,7 @@ import {
import { TextEditor } from '../../field_components';
import { to, from, EDITOR_PX_HEIGHT } from '../shared';

const ignoreFailureConfig: FieldConfig = {
const ignoreFailureConfig: FieldConfig<any, any> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we discussed some of this offline, but seeing it now I am wondering if FieldConfig could be typed as:

interface FieldConfig<ValueType = unknown, T extends FormData = any> {}

Then when it is used on individual fields we can write:

const ignoreFailureConfig: FieldConfig<boolean> = {
 ...
}

I'm not certain how often we will set a type for FieldData when using this type directly, I can see it provides types for the validator, but perhaps the helpers/validators we provide cover a large number of cases already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it makes total sense, I will update it 👍

@sebelga
Copy link
Contributor Author

sebelga commented Oct 2, 2020

Thanks for the review @jloleysens ! I made your suggested change to FieldConfig<T>, thanks for reminding me that 👍

@sebelga
Copy link
Contributor Author

sebelga commented Oct 5, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
indexManagement 1.6MB 1.6MB -15.0B
securitySolution 10.3MB 10.3MB +20.0B
total +5.0B

page load bundle size

id before after diff
esUiShared 303.2KB 304.3KB +1.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sebelga sebelga merged commit 0d378e4 into elastic:master Oct 5, 2020
@sebelga sebelga deleted the form-lib-enhancements branch October 5, 2020 16:09
sebelga added a commit that referenced this pull request Oct 6, 2020
Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Hook form lib] FormSchema does not narrow the index signature
5 participants