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 v17.3.1 #53655

Merged
merged 11 commits into from
Dec 23, 2019
Merged

Upgrade EUI to v17.3.1 #53655

merged 11 commits into from
Dec 23, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Dec 19, 2019

Summary

[email protected][email protected]

  • More form components are now in TypeScript and will be more opinionated about accepted values
  • EuiDataGrid performance boosts 🚀
  • a11y improvements and fixes

17.3.1

Bug fixes

  • Fixed TS types and exports for EuiTextArea and EuiFieldNumber (#2703)

17.3.0

  • Converted EuiFieldNumber to Typescript (#2685)
  • Converted EuiFieldPassword to Typescript (#2683)
  • Converted EuiHighlight to Typescript (#2681)
  • Added data-test-subj property to the EuiCodeEditor component (#2689)
  • Converted EuiTextArea to Typescript (#2695)
  • Converted EuiPage and related child components to TypeScript (#2669)
  • Added annotation glyph (#2691)
  • Added initialWidth and isResizable configurations to EuiDataGrid's columns (#2696)

Bug fixes

  • Reverted removal of toggleOpen method from EuiNavDrawer (#2682)
  • Improved EuiDataGrid update performance (#2676)
  • Fixed EuiDatagrid header top border when configured to have no toolbar (#2619)

17.2.1

[...]

Bug fixes

  • Removed TypeScript definitions in *.test.tsx? files from eui.d.ts (#2673)

17.2.0

  • Improved a11y of EuiNavDrawer lock button state via aria-pressed (#2643)
  • Added new stylesheets for the EUI Amsterdam theme (#2633)
  • Added exports for available types related to EuiDataGrid (#2640)

Bug fixes

  • Improved EuiDataGrid update performance (#2638)
  • Fixed EuiDroppable not accepting multiple children when using TypeScript (#2634)
  • Fixed EuiComboBox from submitting parent form element when selecting options via Enter key (#2642)
  • Fixed EuiNavDrawer expand button from losing focus after click (#2643)
  • Fixed instances of potentially duplicate EuiPopover id attributes (#2667)

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

@elasticmachine merge upstream

@thompsongl thompsongl marked this pull request as ready for review December 20, 2019 18:27
@thompsongl thompsongl requested a review from a team as a code owner December 20, 2019 18:27
@thompsongl thompsongl requested a review from a team December 20, 2019 18:27
@thompsongl thompsongl requested review from a team as code owners December 20, 2019 18:27
@legrego legrego self-requested a review December 20, 2019 18:36
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Security/Spaces changes LGTM. Tested locally.

// @ts-ignore
EuiHighlight,
} from '@elastic/eui';
import { EuiComboBox, EuiComboBoxOptionProps, EuiHealth, EuiHighlight } from '@elastic/eui';
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Approving because this is still better than what it used to be but I am still worried we're potentially masking some bugs and pretending they're data...

@@ -208,7 +208,11 @@ export const dateHistogramOperation: OperationDefinition<DateHistogramIndexPatte
<EuiFlexItem>
<EuiFieldNumber
data-test-subj="lensDateHistogramValue"
value={interval.value}
value={
typeof interval.value === 'number' || interval.value === ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super problematic but seems odd that EUI accepts a number or an empty string... (Am I reading that right?)

I'd expect a type signature more like number | undefined maybe?

Also, does this need a NaN check?

value={
typeof interval.value === 'number' || interval.value === ''
? interval.value
: parseInt(interval.value, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for an EUI update PR but is this really a good spot to do this?

If a non-number has gotten to this point, would it be better to throw an error rather the furthering the rabbit hole of chasing down bad data? Basically, this could lead to subtle bugs that cause people to make mistakes assuming the data is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the options here are to cast value as EuiFieldNumberProps['value'] or attempt to resolve any errors (like so).

Earlier in the script, parseInterval has already transformed interval.value into number | '' like we want. Elsewhere in this file, though, interval.value is expected to be a string.

@@ -186,7 +186,11 @@ export const JsonWatchEditSimulate = ({
<EuiFlexGroup>
<EuiFlexItem>
<EuiFieldNumber
value={scheduledTimeValue}
value={
Copy link
Contributor

Choose a reason for hiding this comment

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

Same discussion questions as above for all of these

@thompsongl
Copy link
Contributor Author

Approving because this is still better than what it used to be but I am still worried we're potentially masking some bugs and pretending they're data...

Agreed, @myasonik. This maintains parity, but does highlight some places where data types should be evaluated

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

App Arch code changes LGTM.

@thompsongl
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@elastic elastic deleted a comment from elasticmachine Dec 23, 2019
@thompsongl thompsongl merged commit e707692 into elastic:master Dec 23, 2019
thompsongl added a commit to thompsongl/kibana that referenced this pull request Dec 23, 2019
* eui to 17.3.0

* eui to 17.3.1

* TS updates

* snapshot updates

* update data-test-subj

Co-authored-by: Elastic Machine <[email protected]>
thompsongl added a commit that referenced this pull request Dec 23, 2019
* eui to 17.3.0

* eui to 17.3.1

* TS updates

* snapshot updates

* update data-test-subj

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 30, 2019
…le-saved-objects

* 'master' of github.com:elastic/kibana: (250 commits)
  Allow chromeless applications to render via non-/app routes (elastic#51527)
  Add server rendering service to enable standalone route rendering (elastic#52161)
  Possibility to filter when testing scripted fields (elastic#35379) (elastic#44220)
  Update maps telemetry mappings to account for recent updates (elastic#53803)
  [Maps] Only show legend when layer is visible (elastic#53781)
  remove use of experimental fs.promises api (elastic#53346)
  [APM] Add log statements for flaky test (elastic#53775)
  [APM] Transaction page throws unhandled exception if transactions doesn't have  `http.request` (elastic#53760)
  Licensing plugin functional tests (elastic#53580)
  [Lens] Disable saving visualization until there are no changes to the document (elastic#52982)
  [Monitoring] Added safeguard for some EUI components (elastic#53318)
  [Vega] Shim new platform - cleanup vega_visualization dependencies (elastic#53605)
  Display changed field formats without requiring hard page refresh. (elastic#53746)
  Upgrade EUI to v17.3.1 (elastic#53655)
  [APM] Fix missing apm indicies (elastic#53541)
  Disable inspector for timelion (elastic#53747)
  Clean up search servie (elastic#53701)
  [Dashboard] Grid: removing double handler (elastic#53707)
  Remove SavedObjectRegistryProvider from codebase (elastic#53455)
  Move ui/courier into data shim plugin (elastic#52359)
  ...
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 8, 2020
* eui to 17.3.0

* eui to 17.3.1

* TS updates

* snapshot updates

* update data-test-subj

Co-authored-by: Elastic Machine <[email protected]>
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