Skip to content

EuiHighlight: converted to Typescript#2681

Merged
chandlerprall merged 5 commits intoelastic:masterfrom
ffknob:convert-EuiHighlight-to-TS
Dec 17, 2019
Merged

EuiHighlight: converted to Typescript#2681
chandlerprall merged 5 commits intoelastic:masterfrom
ffknob:convert-EuiHighlight-to-TS

Conversation

@ffknob
Copy link
Copy Markdown
Contributor

@ffknob ffknob commented Dec 17, 2019 •
edited
Loading

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
Copy Markdown
Collaborator

elasticmachine commented Dec 17, 2019

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
This was referenced Dec 17, 2019
Closed
Open
@chandlerprall chandlerprall self-requested a review December 17, 2019 19:07
chandlerprall
chandlerprall suggested changes Dec 17, 2019
View reviewed changes
Copy link
Copy Markdown
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
);
};

EuiHighlight.propTypes = {
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall Dec 17, 2019

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
Copy Markdown
Contributor Author

@ffknob ffknob Dec 17, 2019

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)

@ffknob
Copy link
Copy Markdown
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
Copy Markdown
Contributor

chandlerprall commented Dec 17, 2019

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.

chandlerprall
chandlerprall suggested changes Dec 17, 2019
View reviewed changes
Copy link
Copy Markdown
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

src/components/highlight/highlight.tsx Outdated
CommonProps & {
children: string;

className?: string;
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall Dec 17, 2019

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

chandlerprall
chandlerprall approved these changes Dec 17, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

chandlerprall commented Dec 17, 2019

Thank you once again, @ffknob !

@thompsongl thompsongl mentioned this pull request Dec 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chandlerprall chandlerprall chandlerprall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

typescript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EuiHighlight needs to get converted to TS

4 participants

@ffknob @elasticmachine @chandlerprall @snide