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

[ML] Migrates ml-form-label to EUI/React. #21059

Merged
merged 4 commits into from
Jul 23, 2018

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 21, 2018

  • Migrates the ml-form-label directive to use EUI/React.
  • Exposes both FormLabel and JsonTooltip as React components from individual files so they can be used in a React context when the wrapping element also has been already ported to React.
  • Adds jests based tests for the FormLabel and JsonTooltip components. They try where possible to make the same assertions like the mocha based tests. The mocha based tests are kept in the code for now so the code gets still tested in a angular based context and as a reference to have the same mocha/jest based tests side by side as a reference for migration.
  • The FormLabel component is done in a way so it supports transclusion in both cases when used with React alone (using the children prop) and angularjs (using a ref callback and angular's transclude()).

Part of #21059.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Great stuff. LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

A couple of comments, otherwise LGTM


// directive for creating a form label including a hoverable icon
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it worth keeping in most of this comment which explains what the component does and just remove the Angular parts?

Copy link
Contributor Author

@walterra walterra Jul 23, 2018

Choose a reason for hiding this comment

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

I adapted the text here in 2073221. I originally copied it to form_label_directive but you're right of course, once the angular part is gone, we need the comment here.

const { labelId, children } = this.props;
return (
<React.Fragment>
<label className="kuiFormLabel" id={`ml_aria_label_${labelId}`} ref={this.labelRef}>{children}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be using the kuiFormLabel class? Is there an eui style we can use instead? If you do change the kuiFormLabel class, the Jest file will need updating too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 0bb4939 I updated the component so it defaults to euiFormLabel but you can override it with another class, for example with kuiFormLabel when used within an angular context (necessary so we don't end up with mixed styles when a form uses both ml-form-label and plain labels using kuiFormLabel).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit d6453fa into elastic:master Jul 23, 2018
@walterra walterra deleted the ml-form-label-react branch July 23, 2018 09:42
walterra added a commit to walterra/kibana that referenced this pull request Jul 23, 2018
- Migrates the ml-form-label directive to use EUI/React.
- Exposes both FormLabel and JsonTooltip as React components from individual files so they can be used in a React context when the wrapping element also has been already ported to React.
- Adds jests based tests for the FormLabel and JsonTooltip components. They try where possible to make the same assertions like the mocha based tests. The mocha based tests are kept in the code for now so the code gets still tested in a angular based context and as a reference to have the same mocha/jest based tests side by side as a reference for migration.
- The FormLabel component is done in a way so it supports transclusion in both cases when used with React alone (using the children prop) and angularjs (using a ref callback and angular's transclude()).
walterra added a commit that referenced this pull request Jul 23, 2018
- Migrates the ml-form-label directive to use EUI/React.
- Exposes both FormLabel and JsonTooltip as React components from individual files so they can be used in a React context when the wrapping element also has been already ported to React.
- Adds jests based tests for the FormLabel and JsonTooltip components. They try where possible to make the same assertions like the mocha based tests. The mocha based tests are kept in the code for now so the code gets still tested in a angular based context and as a reference to have the same mocha/jest based tests side by side as a reference for migration.
- The FormLabel component is done in a way so it supports transclusion in both cases when used with React alone (using the children prop) and angularjs (using a ref callback and angular's transclude()).
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants