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.

Sign up for GitHub

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

Jump to bottom

Convert EuiDescribedFormGroup to typeScript #2810

Merged
merged 7 commits into from
Feb 3, 2020
Merged

Convert EuiDescribedFormGroup to typeScript #2810

merged 7 commits into from
Feb 3, 2020

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Jan 30, 2020 •
edited
Loading

Summary

Closes #2649.

This is basically an attempt to convert the EuiDescribedFormGroup to typescript.

Checklist

  • Check against all themes for compatibility 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

@miukimiu miukimiu changed the title Convert described form group to typeScript Convert EuiDescribedFormGroup to typeScript Jan 30, 2020
@miukimiu miukimiu mentioned this pull request Jan 30, 2020
Merged
9 tasks
thompsongl
thompsongl suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Nice job! A couple things below

Also, rename described_form_group/index.tsx to described_form_group/index.ts

src/components/flex/flex_group.tsx Outdated Show resolved Hide resolved
src/components/form/described_form_group/described_form_group.tsx Outdated Show resolved Hide resolved
src/components/form/described_form_group/described_form_group.tsx Outdated Show resolved Hide resolved
thompsongl
thompsongl suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

These 3 all go together. Should be the last necessary changes.

src/components/form/described_form_group/described_form_group.tsx Outdated Show resolved Hide resolved
src/components/form/described_form_group/described_form_group.tsx Outdated Show resolved Hide resolved
src/components/form/described_form_group/described_form_group.tsx Outdated Show resolved Hide resolved
miukimiu and others added 3 commits January 31, 2020 19:16
@miukimiu @thompsongl
Update src/components/form/described_form_group/described_form_group.tsx
8eec5a5 
Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>
@miukimiu @thompsongl
Update src/components/form/described_form_group/described_form_group.tsx
f529f37 
Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>
@miukimiu @thompsongl
Update src/components/form/described_form_group/described_form_group.tsx
15b72b8 
Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>
cchaos
cchaos approved these changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM!

thompsongl
thompsongl suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Almost! Caught something while checking in Kibana

Need to add the idAria prop to EuiDescribedFormGroupProps: idAria?: string

@miukimiu
Copy link
Contributor Author

miukimiu commented Feb 3, 2020

@thompsongl the idAria was deprecated on #2783.

thompsongl
thompsongl approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Ah I missed that. This is good to go, then

@miukimiu miukimiu merged commit a83c853 into elastic:master Feb 3, 2020
@thompsongl thompsongl mentioned this pull request Feb 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@thompsongl thompsongl thompsongl approved these changes

@chandlerprall chandlerprall Awaiting requested review from chandlerprall

Assignees
No one assigned
Labels
typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiDescribedForm needs to be converted to TS
3 participants
@miukimiu @cchaos @thompsongl