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

Major version dependency upgrades, pt. V #4124

Merged
merged 20 commits into from
Oct 19, 2020
Merged

Major version dependency upgrades, pt. V #4124

merged 20 commits into from
Oct 19, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Oct 8, 2020 •
edited
Loading

Summary

Partial resolution of #3969: EuiMarkdownEditor dependencies

  • react-dropzone
  • Misc. rehype-
  • Misc. remark-
  • unified

Most major version changes were due to the introduction of types (:tada:), but unified did have an API change: the result of .process[Sync]() is now stored in result if an object or contents if a string. See relevant markdown editor changes accounting for both.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs 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

@thompsongl thompsongl added the dependencies Pull requests that update a dependency file label Oct 8, 2020
@thompsongl thompsongl mentioned this pull request Oct 8, 2020
Closed
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

chandlerprall
chandlerprall approved these changes Oct 12, 2020
View reviewed changes
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 typo correction & needs a breaking changelog entry. Tested the markdown editor in the CI's docs build, would be really good to try this out in Kibana to get an expectation of how much effort this will (or won't) be to update to.

src/components/markdown_editor/markdown_format.tsx Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor Author

@chandlerprall I didn't expect this to be breaking. Is it something with the plugin API?

@chandlerprall
Copy link
Contributor

@chandlerprall I didn't expect this to be breaking. Is it something with the plugin API?

That was based off the dependencies list changing, but those would should be transparently handled by npm/yarn and aren't breaking changes. So yeah, just calling out the upgrades in a changelog entry should be good 👍

@thompsongl thompsongl marked this pull request as ready for review October 12, 2020 21:28
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

@thompsongl
Copy link
Contributor Author

Checking on the upgrade path in Kibana. Looks like maybe some more nohoist changes and some types to fix. Will have a resolution accounted for before merging.

@thompsongl
Copy link
Contributor Author

Will have a resolution accounted for before merging.

Need to add "@elastic/eui/remark-parse" and "@elastic/eui/unified" to Kibana's nohoist list.

Type updates are trickier: the return type of getDefaultEuiMarkdownProcessingPlugins no longer matches the expected type of the processingPluginList prop. cc// @chandlerprall

@thompsongl
Copy link
Contributor Author

thompsongl commented Oct 15, 2020 •
edited
Loading

Upgrade path in Kibana is now known. Also confirmed that the plugins written in Kibana still work

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

chandlerprall
chandlerprall approved these changes Oct 16, 2020
View reviewed changes
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.

LGTM!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4124/

@thompsongl thompsongl merged commit b99554e into elastic:master Oct 19, 2020
@thompsongl thompsongl mentioned this pull request Oct 22, 2020
Merged
kshitij86 added a commit to kshitij86/eui that referenced this pull request Nov 29, 2020
@kshitij86 @chandlerprall
Major version dependency upgrades, pt. V (elastic#4124)
b192303 
* eslint; prettier

* react-datepicker eslint

* prettier autofix

* more eslint

* CL

* Revert "CL"

This reverts commit 745d9a0.

* markdown deps

* contents | result update

* Update src/components/markdown_editor/markdown_format.tsx

Co-authored-by: Chandler Prall <[email protected]>

* CL

* update remark2rehype types

* CL

* Update CHANGELOG.md

Co-authored-by: Chandler Prall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thompsongl @kibanamachine @chandlerprall