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

FormControlLayout: Error if append prop is a string #2283

Closed
flash1293 opened this issue Sep 4, 2019 · 7 comments
Closed

FormControlLayout: Error if append prop is a string #2283

flash1293 opened this issue Sep 4, 2019 · 7 comments

Comments

@flash1293
Copy link
Contributor

flash1293 commented Sep 4, 2019

Passing in a string as append to anything building on top of EuiFormControlLayout (EuiFieldText, EuiFieldNumber, ...) is allowed by the typings, but throws an error during runtime because it's always treated as a React element (https://github.com/elastic/eui/blob/master/src/components/form/form_control_layout/form_control_layout.tsx#L154).

@cchaos
Copy link
Contributor

cchaos commented Sep 4, 2019

I think this will be fixed by #2167 , but I'll double-check when that PR is finalized

@flash1293
Copy link
Contributor Author

flash1293 commented Sep 4, 2019

Cool, thanks. I'm not sure whether this PR will fix it, it seems like the problem is that cloneElement is called on the node which won't work for strings. I could be wrong though, didn't run it.

createSideNode(
    node: ReactElement,
    side: 'append' | 'prepend',
    key: React.Key
  ) {
    return cloneElement(node, {
      className: `euiFormControlLayout__${side}`,
      key: key,
    });
  }

@cchaos
Copy link
Contributor

cchaos commented Sep 4, 2019

Ah it looks like ReactElement is of type : string | class. Didn't realize string was allowed. The type should probably therefore be of class?

@flash1293
Copy link
Contributor Author

flash1293 commented Sep 4, 2019

Either that or having a separate branch in the code wrapping a naked string in a span and then apply the class to the span.

@chandlerprall
Copy link
Contributor

It doesn't look like #2167 will address this issue: the type for append/prepend in EuiFormControlLayout is ReactElements which resolves to one of or an array of React.ReactElement - which are the results of any JSX e.g. <div>Hello World</div> (type: string | class is valid here, the string for the preceding example would be div, but any React.ReactElement is cloneable).

The issue is the index.d.ts types for EuiFieldText, EuiFieldNumber, etc are too loose by using the React.ReactNode, instead these should re-use the ReactElements type defined in form_control_layout.tsx

@flash1293
Copy link
Contributor Author

If that fits on the roadmap it would be nice to go the other way and extend form control layout to support all of React.ReactNode - then this would be possible:

<EuiFieldText  value={timeoutValue} append="ms" />

Nothing super important though.

@cchaos
Copy link
Contributor

cchaos commented Mar 18, 2020

prepend and appends do now accept strings and automatically wraps them in a <label>. I think this was fixed with the compressed forms.

@cchaos cchaos closed this as completed Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants