[Control] add labelElement prop #1378

llorca · 2017-07-24T21:14:27Z

Fixes #1351

A new prop must be added for TypeScript support because the existing label prop in React.HTMLAttributes only accepts string values, so tsc rejects JSX elements.

JS consumers can happily pass JSX to the label prop (cuz no compiler to stop them and the code does actually support JSX) but we need to add a new prop to sidestep the typings for TS consumers.

blueprint-bot · 2017-07-24T21:20:59Z

Type label with ReactNode

Preview: documentation
Coverage: core | datetime

cmslewis

Just checked into the typings for ReactNode to make sure strings, numbers, etc., were still supported. Looks like we're good!

//
// React Nodes
// http://facebook.github.io/react/docs/glossary.html
// ----------------------------------------------------------------------

type ReactText = string | number;
type ReactChild = ReactElement<any> | ReactText;

// Should be Array<ReactNode> but type aliases cannot be recursive
type ReactFragment = {} | Array<ReactChild | any[] | boolean>;
type ReactNode = ReactChild | ReactFragment | boolean;

…l-1351

giladgray · 2017-07-25T19:22:40Z

❌ THIS DOES NOT COMPILE. Try actually using it in JSX:

<Checkbox label={<strong>"Gilad Gray"</strong>} defaultIndeterminate />

and you'll see an error:

file: 'file:///Volumes/git/blueprint/packages/core/examples/controlsExample.tsx'
severity: 'Error'
message: 'Type 'Element' is not assignable to type 'string | (string & number) | (string & true) | (string & false) | (string & {}) | (string & React...'.
  Type 'Element' is not assignable to type 'string & (string | number | boolean | any[] | ReactElement<any>)[]'.
    Type 'Element' is not assignable to type 'string'.'
at: '26,27'
source: 'ts'

this is because HTMLAttributes defines label: string | number which does not include JSX.Element. you cannot simply add it to the types because the prop is defined twice, so all types must be compatible.

edit: notice how each type in parens above is (string & X): this means that it really only accepts strings, despite how you've changed the local type here.

@llorca please add labelElement instead, as we discussed previously.

giladgray

❌ see comment

i'm the author now

blueprint-bot · 2017-07-26T00:29:27Z

add example to each Control docs section

Preview: documentation
Coverage: core | datetime

llorca · 2017-07-26T01:06:50Z

packages/core/src/components/forms/controls.tsx

@@ -17,8 +17,11 @@ import * as React from "react";
 import * as Classes from "../../common/classes";
 import { IProps, removeNonHTMLProps } from "../../common/props";
 import { safeInvoke } from "../../common/utils";
+import { HTMLInputProps } from "../../index";
+
+export interface IControlProps extends IProps, HTMLInputProps {


llorca · 2017-07-26T01:09:44Z

packages/core/src/components/forms/controls.tsx

 ];

 /** Base Component class for all Controls */
-export class Control<P extends IControlProps> extends React.Component<React.HTMLProps<HTMLInputElement> & P, {}> {
+export class Control<P extends IControlProps> extends React.Component<P, {}> {


Do we still need <P extends IControlProps> if we already have interfaces extending IControlProps, such as export interface ICheckboxProps extends IControlProps?

yes it enforces that the P is a subclass of IControlProps, so I can't pass IDialogProps, for instance. these two extends are separate things.

llorca · 2017-07-26T01:10:47Z

packages/core/src/components/forms/controls.tsx

@@ -62,6 +79,7 @@ export class Control<P extends IControlProps> extends React.Component<React.HTML
                <input {...inputProps} ref={inputRef} type={type} />
                <span className={Classes.CONTROL_INDICATOR} />
                {this.props.label}
+                {this.props.labelElement}


If both label and labelElement props are provided, do we still render both?

what does the code say?

yes, and label comes first.

@giladgray Apologies, I meant to ask if we actually want to render both. Sounds error-prone to allow both at the same time

why? it's just JSX children, there's no limit. seems more error-prone to only render one.

I just can't imagine a case where setting those two props at the same time would be intentional. Probably a false concern 🤷‍♂️

nor can i, but doing the simple thing works reasonably well in all cases here 🍰

giladgray

love it

cmslewis

Yesss.

cmslewis · 2017-07-28T20:22:20Z

packages/core/examples/controlExamples.tsx

+                <Switch labelElement={<strong>Enabled</strong>} />
+                <Switch labelElement={<em>Public</em>} />
+                <Switch labelElement={<u>Cooperative</u>} defaultChecked />
+                <small>This example uses <code>labelElement</code> to demonstrate JSX labels.</small>


thasner · 2017-08-10T16:53:30Z

curious, why add labelElement instead of extending the typings for label?

llorca · 2017-08-10T17:46:00Z

@thasner scroll up and read the convo, especially this comment: #1378 (comment)

thasner · 2017-08-10T17:47:58Z

👍

Type label with ReactNode

1982d74

llorca requested review from cmslewis and giladgray July 24, 2017 21:14

cmslewis previously approved these changes Jul 24, 2017

View reviewed changes

Merge branch 'master' of github.com:palantir/blueprint into al/contro…

10ca817

…l-1351

giladgray suggested changes Jul 25, 2017

View reviewed changes

Gilad Gray added 2 commits July 25, 2017 17:03

add labelElement prop to support JSX in typescript

0a227b8

add example to each Control docs section

47ecaca

giladgray changed the title ~~[Control] Type label with ReactNode~~ [Control] add labelElement prop Jul 26, 2017

giladgray previously approved these changes Jul 26, 2017

View reviewed changes

llorca commented Jul 26, 2017

View reviewed changes

giladgray approved these changes Jul 28, 2017

View reviewed changes

cmslewis approved these changes Jul 28, 2017

View reviewed changes

llorca merged commit 016f82b into master Jul 28, 2017

llorca deleted the al/control-1351 branch July 28, 2017 21:08

giladgray mentioned this pull request Jul 28, 2017

Prepare Release 1.24.0 #1397

Merged

giladgray mentioned this pull request Aug 8, 2017

[TagInput] values accepts any valid JSX #1419

Merged

giladgray mentioned this pull request Oct 9, 2017

[Core] Callout component! #1694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Control] add labelElement prop #1378

[Control] add labelElement prop #1378

llorca commented Jul 24, 2017 •

edited by giladgray

Loading

blueprint-bot commented Jul 24, 2017

cmslewis left a comment

giladgray commented Jul 25, 2017 •

edited

Loading

giladgray left a comment

blueprint-bot commented Jul 26, 2017

llorca Jul 26, 2017

llorca Jul 26, 2017

giladgray Jul 26, 2017

llorca Jul 26, 2017

giladgray Jul 26, 2017

llorca Jul 26, 2017

giladgray Jul 26, 2017

llorca Jul 27, 2017

giladgray Jul 28, 2017 •

edited

Loading

giladgray left a comment

cmslewis left a comment

cmslewis Jul 28, 2017

thasner commented Aug 10, 2017

llorca commented Aug 10, 2017

thasner commented Aug 10, 2017

[Control] add labelElement prop #1378

[Control] add labelElement prop #1378

Conversation

llorca commented Jul 24, 2017 • edited by giladgray Loading

Fixes #1351

blueprint-bot commented Jul 24, 2017

Type label with ReactNode

cmslewis left a comment

Choose a reason for hiding this comment

giladgray commented Jul 25, 2017 • edited Loading

giladgray left a comment

Choose a reason for hiding this comment

blueprint-bot commented Jul 26, 2017

add example to each Control docs section

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

giladgray Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

giladgray left a comment

Choose a reason for hiding this comment

cmslewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thasner commented Aug 10, 2017

llorca commented Aug 10, 2017

thasner commented Aug 10, 2017

llorca commented Jul 24, 2017 •

edited by giladgray

Loading

giladgray commented Jul 25, 2017 •

edited

Loading

giladgray Jul 28, 2017 •

edited

Loading