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

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

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

AMoo-Miki
Copy link
Collaborator

@AMoo-Miki AMoo-Miki commented Mar 23, 2022

Signed-off-by: Miki miki@amazon.com

Description

  • 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
  • Updates functional tests to use updated selectors.

Issues Resolved

#787 - Bump EUI library to last AL2 release
#1266 - CVE-2022-0686
#1304 - CVE-2022-0691

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 March 23, 2022 01:58
@tmarkley
Copy link
Contributor

You can mention the CVEs that are addressed in the commit as well:

Resolves #1266 - CVE-2022-0686
Resolves #1304 - CVE-2022-0691

@tmarkley tmarkley self-assigned this Mar 24, 2022
@AMoo-Miki AMoo-Miki force-pushed the bump-eui-charts-2 branch 2 times, most recently from 5bf10b4 to 31fc3ad Compare March 25, 2022 21:27
package.json Outdated
@@ -207,12 +207,13 @@
"yauzl": "^2.10.0"
},
"devDependencies": {
"@4lolo/resize-observer-polyfill": "^1.5.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we have to bring this in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Context: We talked about this on the other PR.

The, now unmaintained, original resize-observer-polyfill exports ResizeObserver without exporting ResizeObserverCallback. While that could be mitigated by using @types/resize-observer-browser, it causes a different problem where its exported contentRect conflicts with TypeScript's own definition. @4lolo/resize-observer-polyfill takes care of the contentRect problem.

@AMoo-Miki AMoo-Miki force-pushed the bump-eui-charts-2 branch 4 times, most recently from 70a7588 to 6811c82 Compare March 26, 2022 03:15
@@ -44,7 +44,7 @@ export interface PageProps {

export const Page: React.FC<PageProps> = ({ title = 'Untitled', children }) => {
return (
<EuiPageBody>
<EuiPageBody component="main">
Copy link
Collaborator Author

@AMoo-Miki AMoo-Miki Mar 26, 2022

Choose a reason for hiding this comment

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

The default element produced used to main but is div now. All these component="main" additions are to make it use main.

Comment on lines +153 to +157
<i18n.Context>
<EuiModal {...options} onClose={() => modal.close()}>
<MountWrapper mount={mount} className="osdOverlayMountWrapper" />
</EuiModal>
</i18n.Context>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EuiModal now has EuiOverlayMask built into it.

error={error}
isInvalid={isInvalid}
fullWidth
display={'rowCompressed'}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

compressed is replaced with display={'rowCompressed'}.

Comment on lines +175 to +178
<EuiModal
data-test-subj="devToolsSettingsModal"
className="conApp__settingsModal"
onClose={props.onClose}
Copy link
Collaborator Author

@AMoo-Miki AMoo-Miki Mar 26, 2022

Choose a reason for hiding this comment

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

The next 100 lines are really about removing EuiOverlayMask and re-indenting.

* 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`

* Updates functional tests to use updated selectors.

Signed-off-by: Miki <miki@amazon.com>
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 couple minor questions.

@@ -61,7 +72,6 @@
// sass-lint:disable-block no-important
flex-grow: 0 !important;
flex-basis: auto !important;
margin-right: -$euiSizeXS !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this due to changes in the components?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The theme used to have

.euiSuperDatePicker__flexWrapper--noUpdateButton {
  ...
  max-width: 100%;
}

which necessitated a negative margin-right to reduce the gap between the data selection and the "Refresh" button.

The newer EUI has removed the max-width: 100% to allow for

.euiSuperDatePicker__flexWrapper {
  max-width: calc(100% + #{$euiSizeS});
  ...
}

to be applied here which eliminates the need for the negative margin.

recentNavLinks: RecentNavLink[];
}

export function RecentLinks({ recentNavLinks }: Props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this moved elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The files was deleted because it wasn't being used. It appears that someone forgot to remove this unused file.

@kavilla
Copy link
Member

kavilla commented Mar 27, 2022

Thanks for this @AMoo-Miki,

Majority is good so I wonder if it's worth to merge and do a fast follow. However, the biggest problem I have with this is the UX for the collapsible nav on the left side.

TBH I've not seen a collapsible nav bar with unattached x button and this changes the current experience: which is the hamburger menu icon opens and closes the nav bar. But with this the hamburger menu icon opens the side nav bar and when I click on it again it briefly closes the collapsible nav and then it reopens automatically. While the x button closes it and keeps it close until I re-open it.

What I mean:
Original:
old_hamburger

New:
new_hamburger

Is this expected? Is this achievable to fix in this PR or should we create a follow-up?

Thanks again!

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.

Majority LGTM!

@kavilla
Copy link
Member

kavilla commented Mar 27, 2022

When this merges can we check out this issue: #824

@kgcreative
Copy link
Member

Thanks for this @AMoo-Miki,

Majority is good so I wonder if it's worth to merge and do a fast follow. However, the biggest problem I have with this is the UX for the collapsible nav on the left side.

TBH I've not seen a collapsible nav bar with unattached x button and this changes the current experience: which is the hamburger menu icon opens and closes the nav bar. But with this the hamburger menu icon opens the side nav bar and when I click on it again it briefly closes the collapsible nav and then it reopens automatically. While the x button closes it and keeps it close until I re-open it.

I agree that the behavior is odd here. I also think the detached close button feels strange. My preference would be for the menu button to open / close the nav. Having the "X" is good as an explicit close.

I think at this point this needs to be part of the overall navigation redesign effort. Let's address it together with #569

@AMoo-Miki
Copy link
Collaborator Author

AMoo-Miki commented Mar 28, 2022

Thanks @kavilla and @kgcreative. This odd behavior is due to the outsideClickCloses of EuiFlyout, passed by EuiCollapsibleNav, firing before the click handler of the button and resulting in closing and reopening; outsideClickCloses defaults to true on EuiCollapsibleNav. Setting the prop to false appears to solve the problem but I would like to check it more so I'd prefer not to block this PR.

Raised #1391 to track the fix.

@kavilla kavilla merged commit d56ca22 into opensearch-project:main Mar 28, 2022
@kavilla kavilla linked an issue Apr 5, 2022 that may be closed by this pull request
@tmarkley tmarkley added v2.0.0 ui library Issue or PR related to the UI component library labels Apr 12, 2022
@tmarkley tmarkley added the dependencies Pull requests that update a dependency file label Apr 12, 2022
@tmarkley tmarkley added the cve Security vulnerabilities detected by Dependabot or Mend label Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cve Security vulnerabilities detected by Dependabot or Mend dependencies Pull requests that update a dependency file ui library Issue or PR related to the UI component library v2.0.0
Projects
None yet
4 participants