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

Bumps @elastic/eui to v34.6.0 and @elastic/charts to v31.1.0 #1252

Merged
merged 1 commit into from
Feb 25, 2022
Merged

Bumps @elastic/eui to v34.6.0 and @elastic/charts to v31.1.0 #1252

merged 1 commit into from
Feb 25, 2022

Conversation

AMoo-Miki
Copy link
Collaborator

@AMoo-Miki AMoo-Miki commented Feb 15, 2022 •
edited
Loading

Description

  • Bumps the dependencies on @elastic/eui and @elastic/charts to their the last Apache-2.0-licensed releases.

  • Mitigates changes in @elastic/eui v29.3.2 → v34.6.0

    • v30.0.0: Removal of EuiButtonToggle
    • v30.0.0: Renaming of EuiButtonGroupOption
    • v30.0.0: Requiring legend and isSelected (unless type='multi') props for EuiButtonGroup
    • v30.0.0: Removal of EuiNavDrawerGroup
    • v30.0.0: Removal of compressed prop from EuiFormRow
    • v30.0.0: Removal of displayOnly prop from EuiFormRow
    • v30.0.0: Elimination of the withTitle prop from EuiPopover
    • v30.2.0: Addition of indicator icon for external EuiLink
    • v30.3.0: Test failure due to the restructuring of focus in EuiPopover
    • v31.6.0: Addition of uiOverlayMask directly to EuiModal
    • v31.9.0: Setting default size of “xs” and styling on EuiButtonIcon (partial)
    • v33.0.0: Removal of sh from languages supported by EuiCodeBlock
    • v34.1.0: Changing of the default component of EuiPageBody from main to div

  • Mitigates changes in @elastic/charts v23.2.2 → v31.1.0

    • v29.0.0: Renaming of AnnotationDomainTypes to AnnotationDomainType

Signed-off-by: Miki [email protected]

Issues Resolved

#787

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@AMoo-Miki AMoo-Miki requested a review from a team as a code owner February 15, 2022 23:34
mihirsoni
mihirsoni approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@mihirsoni mihirsoni left a comment

Choose a reason for hiding this comment

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

Thanks for breaking up PRs. LGTM !!!

@kavilla
Copy link
Member

kavilla commented Feb 16, 2022 •
edited
Loading

I am seeing test failures in the functional tests, for example:
node scripts/functional_tests.js --config test/functional/config.js --include ciGroup2

@AMoo-Miki AMoo-Miki mentioned this pull request Feb 16, 2022
Closed
kavilla
kavilla reviewed Feb 18, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Seeing FTR test failings but won't block since it is going to a feature branch.

@tmarkley
Copy link
Contributor

Can we fix the functional tests now so that the issue isn't compounded when we fold in after this?

@AMoo-Miki AMoo-Miki force-pushed the bump-eui-charts branch 3 times, most recently from f0ae306 to f6de6c9 Compare February 24, 2022 20:01
@AMoo-Miki
Copy link
Collaborator Author

Can we fix the functional tests now so that the issue isn't compounded when we fold in after this?

At this point, the only failures are unrelated to the changes made here.

tmarkley
tmarkley suggested changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

I see that you're including commits not rebased to the feature/bump-eui-and-charts branch yet. Can you rebase that branch first?

@AMoo-Miki AMoo-Miki requested a review from tmarkley February 25, 2022 16:48
tmarkley
tmarkley reviewed Feb 25, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few things I'm curious about.

src/core/public/styles/_base.scss Show resolved Hide resolved
src/plugins/data/public/ui/filter_bar/filter_options.tsx Show resolved Hide resolved
tsconfig.base.json
@@ -24,6 +24,7 @@
// includes support for browser APIs
"dom"
],
"skipLibCheck": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to introduce this now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resize-observer-polyfill, via eui.d.ts, exported ResizeObserver without exporting ResizeObserverCallback. The polyfill also exported contentRect which collided with typescript's built-in one.

Copy link
Contributor

Choose a reason for hiding this comment

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

This impacts all declaration files though, right? Can we address this individually instead of skipping the type checking for everything? https://www.typescriptlang.org/tsconfig#skipLibCheck

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is temporary, until I fold in.

  • My original solution, for the folded-in version, uses @juggle/resize-observer but after this PR, I have changed my mind because it is a ponyfill.
  • I have identified @rkesters/resize-observer-polyfill and @4lolo/resize-observer-polyfill as possible options. Since part of the conflict is caused by @types/resize-observer-browser, I have to check if they work.
  • My worst case scenario involves writing my own type-def to be employed via compilerOptions.
  • I remember hearing about some possible effort to upgrade typescript; that would impact change the direction completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay that makes sense. Just as long as we have a plan.

Yes, I upgraded TypeScript to 4.1 in #1230 but I haven't gotten around to resolving the conflicts/test failures yet.

src/plugins/discover/public/application/angular/doc_table/components/table_row/_open.scss Show resolved Hide resolved
src/plugins/discover/public/application/angular/doc_table/components/table_row/open.html Show resolved Hide resolved
src/core/public/styles/_base.scss

/*
Override for default size and style of EuiButtonIcon
Todo: Address via size prop after folding EUI in
Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting for future reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will be handled while folding in

src/plugins/discover/public/application/components/sidebar/discover_field_search.test.tsx Show resolved Hide resolved
src/plugins/home/public/application/components/tutorial/instruction.js Show resolved Hide resolved
src/plugins/home/public/application/components/tutorial/tutorial.js Show resolved Hide resolved
src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx Outdated Show resolved Hide resolved
@AMoo-Miki
Bumps @elastic/eui to v34.6.0 and @elastic/charts to v31.1.0
91f18af 
* Bumps the dependencies on @elastic/eui and @elastic/charts to their the last Apache-2.0-licensed releases.

* Mitigates changes in `@elastic/eui` v29.3.2 → v34.6.0
  * v30.0.0: Removal of `EuiButtonToggle`
  * v30.0.0: Renaming of `EuiButtonGroupOption`
  * v30.0.0: Requiring `legend` and `isSelected` (unless type='multi') props for `EuiButtonGroup`
  * v30.0.0: Removal of `EuiNavDrawerGroup`
  * v30.0.0: Removal of `compressed` prop from `EuiFormRow`
  * v30.0.0: Removal of `displayOnly` prop from `EuiFormRow`
  * v30.0.0: Elimination of the `withTitle` prop from `EuiPopover`
  * v30.2.0: Addition of indicator icon for external `EuiLink`
  * v30.3.0: Test failure due to the restructuring of focus in `EuiPopover`
  * v31.6.0: Addition of `uiOverlayMask` directly to `EuiModal`
  * v31.9.0: Setting default `size` of “xs” and styling on `EuiButtonIcon` (partial)
  * v33.0.0: Removal of `sh` from languages supported by `EuiCodeBlock`
  * v34.1.0: Changing of the default component of `EuiPageBody` from `main` to `div`

* Mitigates changes in `@elastic/charts` v23.2.2 → v31.1.0
  * v29.0.0: Renaming of `AnnotationDomainTypes` to `AnnotationDomainType`

Signed-off-by: Miki <[email protected]>
tmarkley
tmarkley reviewed Feb 25, 2022
View reviewed changes
yarn.lock
resolved "https://registry.yarnpkg.com/url-parse/-/url-parse-1.5.7.tgz#00780f60dbdae90181f51ed85fb24109422c932a"
integrity sha512-HxWkieX+STA38EDk7CE9MEryFeHCKzgagxlGvsdS7WBImq9Mk+PGwiT56w82WI3aicwJA8REp42Cxo98c8FZMA==
url-parse@^1.5.0:
version "1.5.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for reference: this resolves #1266

tmarkley
tmarkley approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Looks great Miki, thanks so much for the effort.

@AMoo-Miki AMoo-Miki merged commit 7927404 into opensearch-project:feature/bump-eui-and-charts Feb 25, 2022
AMoo-Miki added a commit that referenced this pull request Mar 2, 2022
@AMoo-Miki
Bumps @elastic/eui to v34.6.0 and @elastic/charts to v31.1.0 (#1252)
752dfce 
* Bumps the dependencies on @elastic/eui and @elastic/charts to their last Apache-2.0-licensed releases.

* Mitigates changes in `@elastic/eui` v29.3.2 → v34.6.0
  * v30.0.0: Removal of `EuiButtonToggle`
  * v30.0.0: Renaming of `EuiButtonGroupOption`
  * v30.0.0: Requiring `legend` and `isSelected` (unless type='multi') props for `EuiButtonGroup`
  * v30.0.0: Removal of `EuiNavDrawerGroup`
  * v30.0.0: Removal of `compressed` prop from `EuiFormRow`
  * v30.0.0: Removal of `displayOnly` prop from `EuiFormRow`
  * v30.0.0: Elimination of the `withTitle` prop from `EuiPopover`
  * v30.2.0: Addition of indicator icon for external `EuiLink`
  * v30.3.0: Test failure due to the restructuring of focus in `EuiPopover`
  * v31.6.0: Addition of `uiOverlayMask` directly to `EuiModal`
  * v31.9.0: Setting default `size` of “xs” and styling on `EuiButtonIcon` (partial)
  * v33.0.0: Removal of `sh` from languages supported by `EuiCodeBlock`
  * v34.1.0: Changing of the default component of `EuiPageBody` from `main` to `div`

* Mitigates changes in `@elastic/charts` v23.2.2 → v31.1.0
  * v29.0.0: Renaming of `AnnotationDomainTypes` to `AnnotationDomainType`

Signed-off-by: Miki <[email protected]>
@AMoo-Miki AMoo-Miki mentioned this pull request Mar 25, 2022
Merged
7 tasks
@joshuarrrr joshuarrrr mentioned this pull request Mar 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavilla kavilla kavilla left review comments

@tmarkley tmarkley tmarkley approved these changes

@mihirsoni mihirsoni mihirsoni approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@AMoo-Miki @kavilla @tmarkley @mihirsoni