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 to [email protected] [table typescript types] #52688

Merged
merged 19 commits into from
Dec 18, 2019

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Dec 10, 2019

Summary

The two biggest changes here are icon snapshot updates (for accessibility, elastic/eui#2554) and typescript definitions for EuiBasicTable and EuiInMemoryTable (elastic/eui#2428).

When unifying Kibana's usage of eui tables with the new types, I avoided changes to run-time code, always preferring to mess only with the type definitions, usages, and in some cases avoiding the confusion with a // @ts-ignore. The primary differences are:

  • EUI table types have a generic parameter which describes the Item each row represents
  • Column rendering callbacks now specify that generic Item, and field rendering ensures the specified field exists on the item.
  • Pagination and sorting have stricter types, again based on Item. This, combined with TypeScript's lack of usage-based inference, led to a number of additional as const.

In addition, some apps (SIEM in particular) have defined a Direction enum of ASC & DESC which caused problems in the sorting callbacks which expect the enum value, but the EUI table only guarantees a string literal of 'asc' | 'desc' - TS enum values are nominally typed, as demonstrated by this typescript playground. I resolved those by updating the onSort callback to allow Direction | 'asc' | 'desc'.

17.1.2

Bug fixes

  • Fixed EuiCodeEditor custom mode file error by initializing with existing mode (#2616)
  • Removed EuiIcon default titles (#2632)

17.1.1

Bug fixes

  • Fixed screenreader text in EuiTreeView and added truncation (#2627)

17.1.0

  • Added an optional key property inside the options prop in EuiSelectableList component (#2608)
  • Added toolbarAdditionalControls prop to EuiDataGrid to allow for custom buttons in the toolbar (#2594)
  • Added TypeScript definitions for EuiBasicTable, EuiInMemoryTable, and related components (#2428)
  • Updated logoSecurity and appSecurityAnalytics icons (#2613)

Bug fixes

  • Fixed UX/focus bug in EuiDataGrid when using keyboard shortcuts to paginate (#2602)
  • Fixed EuiIcon accessibility by adding a title prop and a default aria-label (#2554)
  • Fixed EuiDataGrid's in-memory sorting of numeric columns when the cell data contains multiple digit groups (#2603)
  • Improved pagination in EuiBasicTable. paginationBar is hidden when there is no data and EuiPagination is displayed even when there is only one page (#2598)
  • Fixed react-dom warning when EuiPopover was unmounted before calls to setState (#2614)

@chandlerprall chandlerprall changed the title [DRAFT] Upgrade to [email protected] [table typescript types] [DRAFT] Upgrade to [email protected] [table typescript types] Dec 12, 2019
@chandlerprall chandlerprall marked this pull request as ready for review December 12, 2019 19:51
@chandlerprall chandlerprall requested review from a team as code owners December 12, 2019 19:51
@chandlerprall chandlerprall changed the title [DRAFT] Upgrade to [email protected] [table typescript types] Upgrade to [email protected] [table typescript types] Dec 12, 2019
@legrego legrego self-requested a review December 12, 2019 20:00
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

@elastic/kibana-app-arch LGTM

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for platform changes

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/transform changes LGTM. Thanks for this update, this should allow us to get rid of our custom types in a follow up!

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 changes LGTM

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

infra changes LGTM - thank you!

@@ -169,8 +169,8 @@ export const AnomaliesTable: React.FunctionComponent<{
);
};

const StyledEuiBasicTable = euiStyled(EuiBasicTable)`
const StyledEuiBasicTable: typeof EuiBasicTable = euiStyled(EuiBasicTable as any)`
Copy link
Member

Choose a reason for hiding this comment

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

Not pretty but I couldn't find a better way either 🤷‍♀️

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link
Contributor

@smith smith 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 look good. 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@chandlerprall chandlerprall merged commit 068f3ff into elastic:master Dec 18, 2019
@chandlerprall chandlerprall deleted the eui-17.1.0 branch December 18, 2019 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants