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

[Docs] Convert all Elastic Charts docs pages to Typescript #7277

Merged
merged 19 commits into from
Oct 12, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 10, 2023

Summary

Turns out there's a whole lot of out of date APIs that weren't getting caught in our automated Elastic Charts updates, because the files were in JS 😬 This should help our docs keep more up to date with charts APIs.

QA

General checklist

N/A, docs only

- convert `PropTypes` to TS types

- Inline events to avoid having to type them explicitly
@cee-chen cee-chen added documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog tech debt labels Oct 10, 2023
- convert to new API
- some where moved to theme settings instead, others just shouldn't have been there at all in the first place?
- mixed logic is a bit of a headache try to simplify it via a conditional generic
- moving onChange callback to inline removes the need for typing the params explicitly
- which removes the need to type the params explicitly
- remove unused text/title props

- make `description` correctly conditional, and add more spacing to the affected card to make up for it
- including obj iteration microperf improvements
- charts lib appears to work now with react 18
@cee-chen
Copy link
Member Author

@nickofthyme Hey hey! Would appreciate your help with the following:

The rest of the commits are pretty straightforward type updates and I'm fairly confident in them, but the above ones are props/APIs owned by your team that I kinda guesstimated by either looking through your changelog or repo. In particular I'm not confident in GoalChart.config (I think that just went away?) or Settings.tooltip="none" (also not sure where that went?)

@cee-chen cee-chen marked this pull request as ready for review October 10, 2023 23:14
@cee-chen cee-chen requested a review from a team as a code owner October 10, 2023 23:14
@nickofthyme
Copy link
Contributor

Oh no 😞, not too surprised. Yes, happy to take a look!

Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

@cee-chen These changes look great!

We really appreciate you tackling this. All the changes in the commits you listed look fine, just a few tweaks, see 9c1c992.

Sorry again for the poor upkeep, now it should be a lot easier to identify breaking changes.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@cee-chen
Copy link
Member Author

Huzzah!! Thank you for the fixes Nick, you rock!

@cee-chen cee-chen merged commit d8d4a3a into elastic:main Oct 12, 2023
7 checks passed
@cee-chen cee-chen deleted the charts-docs-types branch October 12, 2023 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants