LOOM-1540: tsx migration for splitinput and inputField components

[Parisistan]

Giving splitInputs some typescript LOVE.
Add form test for splitInput

FYI: Had to ignore a lint for one of the test because otherwise we'd need to write a recursive promise chain to do it and it's unnecessarily complicated for this simple test. 🙂

Remember to include the following changes:

• Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
• README.md (If you have created a new component)
• Component README.md
• Tests
• Accessibility tests
  • The following checks were performed:
    • Ability to navigate using a keyboard only
    • Zoom functionality (Deque University explanation):
      • The page SHOULD be functional AND readable when only the text is magnified to 200% of its initial size
      • Pages must reflow as zoom increases up to 400% so that content continues to be presented in only one column i.e. Content MUST NOT require scrolling in two directions (both vertically and horizontally)
    • Ability to navigate using a screen reader only
• Storybook examples created/updated
• For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

[Parisistan]

English Grammar 😸

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[github-actions]

	Warnings
⚠️	Package source files (e.g. packages/package-name/src/Component.js) were updated, but snapshots weren't. Have you checked that the tests still pass?
⚠️	Package source files (e.g. packages/package-name/src/Component.tsx) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against b1c3647

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[olliecurtis]

Usually this is placed at the end of the component so as not to accidently override any consumer provided props/overrides.

Additional objects should always be placed on the component last

[Parisistan]

This was a conscious decision to put rest there so we override the below components based on how we handle them. rest at the end was making unwanted changes.

[olliecurtis]

Ok, maybe we need to check this out more, as rest should always come last. Could you clarify the unwanted change this would be making?

Could it be something we are passing in from the examples that overrides this

[Parisistan]

As far as I understand we want to handle props as we have described in each function for:

          onInput={this.handleOnChange}
          onKeyDown={this.handleOnKeyDown}
          onPaste={this.handleOnPaste}
          onFocus={(e: FocusEvent<HTMLInputElement>) => this.handleOnFocus(e, index)}
          onChange={this.handleOnChange}

But if we use rest at the end, it overwrites our functions and how we want to handle our props.

[metalix2]

So this Component has all these custom handle event functions for managing behaviour.
If we allow parent onChange functions to override this components onEvent behaviour it could break.. Though I suppose it's up to the consumer... I think I may be wrong about this and rest can be at the end.

[Parisistan]

Alright ... If I move {...rest} down, it means they are overwiring the handle fuctions which makes the handle functions irrelevant and creating error below:

How do we handle that?

[metalix2]

That's only if the consumer specifies the on{Event} functions. Which I suppose the onChange is a prop. You can define a default value for onChange? or is it that onChange is set in handleOnChange?

[metalix2]

If we defin onChange above then this error should go away? and we can specify if onChange ? onChange : this.handleOnChange

[metalix2]

Well turns out this prop was poorly named as it's purpose wasn't to be passed down to the input field it's to just give the consumer a way to add their function to the existing logic.

[olliecurtis]

Could you possibly take a look at moving away from the toMatchSnapshot() and making it so it does better testing?

[Parisistan]

Could you please elaborate and provide examples @olliecurtis ?

[olliecurtis]

Sure thing! I mean changing the tests to be more like we have now in the form-test.tsx that you have added in the PR: https://github.com/Skyscanner/backpack/pull/3565/files#diff-f889630b8387e0467cccc8992ea53a16a9fac15ae0fceaf69f9ce9021ca35f95R58

Or like this: https://github.com/Skyscanner/backpack/blob/main/packages/bpk-component-skeleton/src/BpkSkeleton-test.tsx#L28

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.

[olliecurtis]

With the new change to this property this will change the contract with consumers, so we will need to move the label on this PR to be major :)

[github-actions]

Visit https://backpack.github.io/storybook-prs/3565 to see this build running in a browser.