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

EuiHighlight: converted to Typescript #2681

Merged
merged 5 commits into from
Dec 17, 2019

Conversation

ffknob
Copy link
Contributor

@ffknob ffknob commented Dec 17, 2019

Summary

Closes #2666
Converted EuiHighlight to Typescript.

Checklist

  • Check against all themes for compatability in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ffknob ffknob changed the title EuiHighlight: translated to Typescript EuiHighlight: converted to Typescript Dec 17, 2019
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Couple changes requested, also needs a CHANGELOG.md entry

src/components/highlight/highlight.tsx Outdated Show resolved Hide resolved
);
};

EuiHighlight.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these propTypes, they will be generated automatically from the TS definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the goal is to move all propTypes to the component's interface? (just to be sure before I take the other convertions)

src/components/highlight/highlight.tsx Show resolved Hide resolved
@ffknob
Copy link
Contributor Author

ffknob commented Dec 17, 2019

  • CHANGELOG: added (thought code refactory was not to be put there, my mistake)
  • HTMLSpanElement: corrected
  • children: included in the component's Props as string

One doubt about propTypes as general (so I can take on the other convertions): should I completly get rid of it and move all attributes to the Props interface?

@chandlerprall
Copy link
Contributor

One doubt about propTypes as general (so I can take on the other convertions): should I completly get rid of it and move all attributes to the Props interface?

Yes, the React propTypes duplicate the information provided by the typescript definition, so we don't want to include them in the components. However, it is useful to provide the proptypes when components are used in non-TS projects, so we generate them (based on the TS definitions) at build time. Our docs also use the resulting proptypes to populate any props tabs.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One final nit pick, otherwise this looks good

CommonProps & {
children: string;

className?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

className is already provided by CommonProps and doesn't need to be re-specified, common.ts#L10

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, pulled and tested locally

@chandlerprall chandlerprall merged commit e811631 into elastic:master Dec 17, 2019
@chandlerprall
Copy link
Contributor

Thank you once again, @ffknob !

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.

EuiHighlight needs to get converted to TS
4 participants