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

Upgrades EUI to 9.x #32009

Merged
merged 16 commits into from
Mar 7, 2019
Merged

Upgrades EUI to 9.x #32009

merged 16 commits into from
Mar 7, 2019

Conversation

jasonrhodes
Copy link
Member

@jasonrhodes jasonrhodes commented Feb 26, 2019

Summary

This PR replaces #31935 and #31801

UPDATE: The latest EUI is now at 9.x, so this PR upgrades to 9.0.x

@jasonrhodes jasonrhodes mentioned this pull request Feb 26, 2019
@@ -31,9 +31,7 @@ exports[`Home component should render 1`] = `
<SetupInstructionsLink />
</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer
Copy link
Member Author

Choose a reason for hiding this comment

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

@chandlerprall see comments from @sqren and @nreese here: #31935 (comment)

Has the default size for this component changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Default size remains the same, but has been moved from defaultProps to being applied at destructuring in the function component's signature, so React is not aware of the default.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Mar 1, 2019

Have you seen #32291? Looks like there is another PR attempting to bump EUI to 9.0.1

@jasonrhodes jasonrhodes changed the title Upgrades EUI to 8.x Upgrades EUI to 9.x Mar 1, 2019
@jasonrhodes jasonrhodes mentioned this pull request Mar 1, 2019
7 tasks
@jasonrhodes jasonrhodes marked this pull request as ready for review March 1, 2019 13:18
@jasonrhodes jasonrhodes requested review from a team as code owners March 1, 2019 13:18
@jasonrhodes
Copy link
Member Author

@chandlerprall @thompsongl I've removed all of the infra duplicates and it seems like the only error left has to do with some EuiProgress component wrapped in a styled-components wrapper here: x-pack/plugins/infra/public/components/logging/log_text_stream/loading_item_view.tsx

ping @skh @elastic/infrastructure-ui

@azasypkin
Copy link
Member

azasypkin commented Mar 1, 2019

@maryia-lapata @jasonrhodes Whoever drags this over a finish line, please don't forget to create follow-up PR to cover #31480 (comment) (this time it'd be better to have a separate follow-up so that we can easily backport a part of it to 6.7 and 7.0 as well). It's something that should always come with EUI upgrades now.

I'm not sure if we have "tokens changelog" section in the main EUI change logs already, but if not we should rely on https://elastic.github.io/eui/#/package/i18n-tokens for this particular upgrade.

Feel free to reach out if you have any questions.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chandlerprall
Copy link
Contributor

I'm not sure if we have "tokens changelog" section in the main EUI change logs already

Not yet

@jasonrhodes
Copy link
Member Author

Update: I've read through styled-components docs and have thus far not been able to figure out how to get around the problem mentioned here: #32009 (comment)

We may need to use // @ts-ignore and let the infra team work with the EUI team to circle back on this.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes
Copy link
Member Author

Now we're getting snapshot failures, which probably explains why the other 9.x PR had about 40-50 more file changes than this one. Heading out for the weekend, will check back on what's going on Monday.

@jasonrhodes
Copy link
Member Author

jasonrhodes commented Mar 4, 2019

I've updated snapshots. Here are the main updates:

  • rgb colors become hex
  • labelType=label is added to EuiFormRow (default changed?)
  • everything that asserts on the results of the status() function in this file (plugins/index_management/jest/components/index_table.test.js) has "status" prepended to the resulting snapshotted text
    • "clearing cache..." becomes "statusclearing cache..."
    • "open" becomes "statusopen"
    • etc
  • <EuiFlyoutHeader> -- hasBorder={false} disappears (default changed?)
  • <EuiTableRowCell> gains new mobileOptions.show = true prop
  • <td data-header="X"> is replaced with <div class="euiTableRowCell__mobileHeader euiTableRowCell--hideForDesktop">X</div> (e.g. plugins/remote_clusters/public/sections/remote_cluster_list/remote_cluster_table/remote_cluster_table.test.js)
  • form row labels are now enclosed in divs (e.g. plugins/remote_clusters/public/sections/components/remote_cluster_form/remote_cluster_form.test.js)
  • giant svg diff here: plugins/apm/public/components/app/ErrorGroupOverview/List/test/List.test.tsx
  • EuiProgress no longer specifies position: static (plugins/apm/public/components/shared/ImpactBar/test/ImpactBar.test.js)

@jasonrhodes
Copy link
Member Author

There are a few failing tests in x-pack, that I didn't feel comfortable updating. It seems like the EuiInMemoryTable is preprending "Field" to the field items? Not sure.

 FAIL  plugins/rollup/public/crud_app/sections/job_list/detail_panel/detail_panel.test.js (6.138s)
  ● <DetailPanel /> › job detail › terms tab content › should list the Job terms fields

    expect(received).toEqual(expected)

    Difference:

    - Expected
    + Received

      Array [
    -   "Dest",
    -   "Carrier",
    -   "DestCountry",
    +   "FieldDest",
    +   "FieldCarrier",
    +   "FieldDestCountry",
      ]

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chandlerprall
Copy link
Contributor

Checked on a few of Jason's noted changes to be safe -

labelType=label is added to EuiFormRow (default changed?)

labelType is a new prop, label is the default value (and mimics the old, prop-less behaviour)

everything that asserts on the results of the status() function in this file (plugins/index_management/jest/components/index_table.test.js) has "status" prepended to the resulting snapshotted text

status is added in a always-present-but-CSS-hidden-on-desktop div, the snapshot is picking up this extra text

-- hasBorder={false} disappears (default changed?)

EuiFlyoutHeader was converted to TypeScript, where we remove defaultProps in favor of adding destructing defaults. The value is the same, but not exposed directly to React now.

EuiProgress no longer specifies position: static

Same as EuiFlyoutHeader, component was converted to TS and React no longer sees the defaults (position default is still static)

@chandlerprall
Copy link
Contributor

chandlerprall commented Mar 4, 2019

The detail_panel.test.js difference is the same thing that happened for index_table.test.js and its status text - there is a mobile-only div saying Field - the snapshot is fine to update.

@chandlerprall
Copy link
Contributor

I have a commit (chandlerprall@2ea1362) that fixes the test. Seems I can't push to Jason's branch, nor make a PR against his branch (?)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chandlerprall
Copy link
Contributor

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

LGREATTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML related changes LGTM.

@jasonrhodes jasonrhodes merged commit ae754ac into elastic:master Mar 7, 2019
@jasonrhodes jasonrhodes deleted the eui-upgrade-8.x branch March 7, 2019 12:03
jasonrhodes added a commit to jasonrhodes/kibana that referenced this pull request Mar 7, 2019
* Fixed a simple argument bug and removed infra date picker EUI types

* Fixes for EUI date picker types

* eui_8.0.0

* fix type errors in query_bar

* Small changes for EUI types

* Updates EUI to 9.0.0 and removes @types/react-datepicker as it now ships with EUI

* Updates to EUI 9.0.1 and removes duplicate types in infra eui.d.ts

* ts-ignore applied to ongoing type error with styled components and EUI

* Changes EuiProgress props to avoid TS errors

* Updates EUI 9.0 snapshots

* Updates kibana root snapshots for EUI 9.0 upgrade

* Update detail_panel.test.js for EUI changes

* Updated functioanl and unit tests to properly inspect EuiTableRowCell rendered values

* Fix docs_level_security_roles.js func tests

* Update EUI to 9.0.2

* Fixed failing snapshot for EUI icon default prop
jasonrhodes added a commit that referenced this pull request Mar 7, 2019
* Fixed a simple argument bug and removed infra date picker EUI types

* Fixes for EUI date picker types

* eui_8.0.0

* fix type errors in query_bar

* Small changes for EUI types

* Updates EUI to 9.0.0 and removes @types/react-datepicker as it now ships with EUI

* Updates to EUI 9.0.1 and removes duplicate types in infra eui.d.ts

* ts-ignore applied to ongoing type error with styled components and EUI

* Changes EuiProgress props to avoid TS errors

* Updates EUI 9.0 snapshots

* Updates kibana root snapshots for EUI 9.0 upgrade

* Update detail_panel.test.js for EUI changes

* Updated functioanl and unit tests to properly inspect EuiTableRowCell rendered values

* Fix docs_level_security_roles.js func tests

* Update EUI to 9.0.2

* Fixed failing snapshot for EUI icon default prop
weltenwort added a commit to weltenwort/kibana that referenced this pull request Mar 18, 2019
…ic#33152)

This fixes a problem introduced with elastic#32009, in which the meaning of the `isLoading` property for the `<ProgressEntry>` component was inverted.
weltenwort added a commit that referenced this pull request Mar 18, 2019
This fixes a problem introduced with #32009, in which the meaning of the `isLoading` property for the `<ProgressEntry>` component was inverted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants