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

Upgrade EUI to 14.7.0 #49110

Merged
merged 5 commits into from
Oct 24, 2019
Merged

Upgrade EUI to 14.7.0 #49110

merged 5 commits into from
Oct 24, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Oct 23, 2019

Most of the changes are from updated icons: ifraApp became metricsApp, and loggingApp became logsApp.


14.7.0

  • Converted EuiRadio and EuiRadioGroup to TypeScript (#2438)
  • Improved a11y in EuiImage (#2447)
  • Made EuiIcon a PureComponent, to speed up React re-render performance (#2448)
  • Added ability for EuiColorStops to accept user-defined range bounds (#2396)
  • Added external prop to EuiLink (#2442)
  • Added disabled state to EuiBadge (#2440)
  • Changed EuiLink to appear non interactive when passed the disabled prop and an onClick handler (#2423)
  • Added minimize glyph to EuiIcon (#2457)

Bug fixes

  • Reenabled width property for EuiTable cell components (#2452)
  • Fixed EuiNavDrawer collapse/expand button height issue
    (#2463)

14.6.0

  • Added new updated infraApp and logsApp icons. (#2430)

Bug fixes

  • Fixed missing misc. button and link type definition exports (#2434)
  • Strip custom semantics from EuiSideNav (#2429)

@thompsongl thompsongl added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes EUI v7.6.0 labels Oct 23, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thompsongl thompsongl marked this pull request as ready for review October 23, 2019 22:24
@thompsongl thompsongl requested review from a team as code owners October 23, 2019 22:24
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 changes LGTM

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

APM changes LGTM

@@ -43,7 +43,7 @@ export function infra(kibana: any) {
defaultMessage: 'Explore your metrics',
}),
icon: 'plugins/infra/images/infra_mono_white.svg',
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpicking] should we rename the SVGs as well to keep the naming consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave that decision up to you all; I'm not sure how much infra-to-metrics language conversion is planned for the app. The underlying image actually changed during the switch from infraApp to metricsApp. Plus, that name is part of the EUI API, so there is some necessity for EUI to align with the direction.

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.

Code, package, and yarn.lock changes LGTM

@thompsongl thompsongl merged commit 9e58b27 into elastic:master Oct 24, 2019
thompsongl added a commit to thompsongl/kibana that referenced this pull request Oct 24, 2019
* eui 14.7.0

* icon updates

* snapshot updates

* storyshot updates

* remove config addition
thompsongl added a commit that referenced this pull request Oct 24, 2019
* eui 14.7.0

* icon updates

* snapshot updates

* storyshot updates

* remove config addition
@clintandrewhall
Copy link
Contributor

@thompsongl @chandlerprall Bummer... this change ended up breaking Canvas assets. (#50733) The Storybook Jest tests caught the change, but the snapshots were simply regenerated, rather than confirmed through the Storybook tool. I'm also a bit concerned this PR was only open for a day, and Canvas isn't a sign-off on the PR...?

It's awesome that #43529 is coming, which will make Storybook more mainstream, so it'll be easy to visually confirm changes. Aside from that, how can we prevent this kind of issue moving forward?

cc: @poffdeluxe

@chandlerprall
Copy link
Contributor

Thank you for bringing this up! I want to shed a little light on what happened & our process, and then go from there.

  1. the EUI change that broke canvas assets was made too flippantly, without consideration for existing uses
  2. our upgrade path includes running jest with the update snapshots flag, and then manually checking the updated files. Looking at the diff on this PR again, the change in asset.examples.storyshot should have been caught and verified but was not
  3. I share your concern in skipping any team's approval on these. We've taken the approach that - while we appreciate and prefer getting those approvals - if one was triggered solely by snapshot changes and we can account for that change in EUI's changelog, then we will proceed after a short wait. Depending on urgency (either real or felt), this could be same day or a few days later.

Given 1&2, we had a couple points in time where we could have caught this but didn't, and I think this has to be expected in any manual process - though we can and should take steps to limit it further. Some further thoughts/questions:

  • can jest be configured to not auto-update storyshots / can there be a forcing function to use storybook to confirm these changes
  • we should follow-up to get a team involved (via that team's slack channel?) before merging
  • ? open to any ideas, especially if it can be automated ?

@clintandrewhall
Copy link
Contributor

@chandlerprall Gotcha... given the constraints/considerations, I think integrating Percy into our Storybook setup(s) will make this a ton easier to confirm moving forward. I also think encouraging snapshot tests be generated via Storybook examples, (using #43529), rather than in pure code, we can make it much easier for EUI and others to visually confirm passive changes.

In the short term, I think if jest is run with --update out of the gate, we're just going to have to live with a few missteps from time-to-time.

@thompsongl
Copy link
Contributor Author

Apologies for the misstep here, @clintandrewhall. Definitely some boxes left unchecked in this particular PR when it came to preventing breaks.

Percy integration would be hugely helpful for EUI upgrade PRs. In the meantime, delaying the merge for a while and making sure your team has a chance to review is the minimum we can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EUI release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants