Skip to content

TS updates#34

Merged
cchaos merged 4 commits intocchaos:chore/decouple_button_contentfrom
thompsongl:chore/decouple_button_content
Jul 14, 2020
Merged

TS updates#34
cchaos merged 4 commits intocchaos:chore/decouple_button_contentfrom
thompsongl:chore/decouple_button_content

Conversation

@thompsongl
Copy link
Copy Markdown

@thompsongl thompsongl commented Jul 13, 2020

disabled is not available on a and label, but it looks like the styling is handled. I didn't visually test, though

@thompsongl thompsongl mentioned this pull request Jul 13, 2020
Merged
3 tasks
@cchaos
Copy link
Copy Markdown
Owner

cchaos commented Jul 13, 2020

Ahhh... hmmm.

So I was hoping that EuiButtonDisplay would try to be as element agnostic as possible so that the consuming component, EuiButton, would be passing the appropriate combination of props, ie disabled to a <button>. Is it possible to move the logic you added primarily to EuiButton instead? That way then element could easily be changed to just string.

@thompsongl
Copy link
Copy Markdown
Author

thompsongl commented Jul 13, 2020

I can take another pass move the logic around.
But I think at some point there will have to be a check to see what kind of element element is so that the ref can be correctly cast.

@cchaos
Copy link
Copy Markdown
Owner

cchaos commented Jul 13, 2020

Yeah I think the ref is the main trouble spot too.

@thompsongl
Copy link
Copy Markdown
Author

thompsongl commented Jul 13, 2020

Yeah I think the ref is the main trouble spot too.

Looking again, if element needs to be even more generic, I'm not sure we can actually type it correctly and would // @ts-ignore the return. That's what happens inEuiFlexItem where we do the same kind of thing.

@thompsongl
Copy link
Copy Markdown
Author

thompsongl commented Jul 13, 2020

At some point ref does need to be cast, so the choices are either define a set of elements and use logic to pass the correct props, or allow any element and ignore the type check.

@cchaos
Copy link
Copy Markdown
Owner

cchaos commented Jul 13, 2020

Since EuiButtonDisplay is an internal component only, I'd say let's TS ignore the ref/return in THAT component, but then ensure that EuiButton is appropriately assigning props to HTML element

cchaos
cchaos reviewed Jul 14, 2020
View reviewed changes
src/components/button/button.tsx

export interface EuiButtonDisplayProps extends EuiButtonProps {
element: 'a' | 'button' | 'label';
element: keyof JSX.IntrinsicElements;
Copy link
Copy Markdown
Owner

@cchaos cchaos Jul 14, 2020

Choose a reason for hiding this comment

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

Out of curiosity, what is this/does this mean?

Copy link
Copy Markdown
Owner

@cchaos cchaos Jul 14, 2020

Choose a reason for hiding this comment

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

Also, would React.createElement help here in any way?

Copy link
Copy Markdown
Author

@thompsongl thompsongl Jul 14, 2020

Choose a reason for hiding this comment

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

keyof JSX.IntrinsicElements

It will allow for basically any HTML element

Also, would React.createElement help here in any way?

That's an interesting idea. Let me try it out; I'm not actually sure what TS will do it.

@cchaos cchaos merged commit dd949b8 into cchaos:chore/decouple_button_content Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cchaos cchaos cchaos left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thompsongl @cchaos