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

[Discover] Replace doc viewer table with EuiInMemoryTable #86757

Closed
kertal opened this issue Dec 22, 2020 · 9 comments · Fixed by #102149
Closed

[Discover] Replace doc viewer table with EuiInMemoryTable #86757

kertal opened this issue Dec 22, 2020 · 9 comments · Fixed by #102149
Assignees
Labels
Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@kertal
Copy link
Member

kertal commented Dec 22, 2020

One piece of legacy to remove, the doc viewer table using bootstrap of Discover should be replaced with EuiInMemoryTable

Other small thing I noticed in here (and can wait) is that we really need to replace the doc viewer table that shows in the expanded flyout to use an EuiTable. Right now it's using Bootstrap, and we're looking to remove that next minor.

Originally posted by @snide in #67259 (comment)

Here's the very quick mocking code for this, adapted in

https://github.com/elastic/kibana/blob/65251051fdcb948bfb0bc03ad211b851bdb877bd/src/plugins/discover/public/application/components/table/table.tsx

const usedColumns = [
    {
      field: 'action',
      name: 'Action',
      width: 100,
      sortable: false,
      render: () => (
        <>
          <EuiButtonIcon
            aria-label={i18n.translate('discover.docViews.table.filterForValueButtonAriaLabel', {
              defaultMessage: 'Filter for value',
            })}
            className="kbnDocViewer__actionButton"
            data-test-subj="addInclusiveFilterButton"
            iconType={'plusInCircle'}
            iconSize={'s'}
          />
          <DocViewTableRowBtnFilterRemove disabled={false} onClick={() => void 0} />
          <DocViewTableRowBtnToggleColumn active={true} onClick={() => void 0} />
          <DocViewTableRowBtnFilterExists disabled={false} onClick={() => void 0} />
        </>
      ),
    },
    {
      field: 'field',
      name: 'Field',
      sortable: true,
      truncateText: false,
      render: (field) => <FieldName fieldName={field} fieldType={'string'} />,
    },
    {
      field: 'value',
      name: 'Value',
    },
  ];

  return (
    <EuiInMemoryTable
      items={table}
      columns={usedColumns}
      pagination={true}
    />
  );

Looks like this:

Bildschirmfoto 2021-06-10 um 21 58 55

Requirements

  • It should work like the legacy table in terms of functionality, no truncation of values
  • Only pagination should be added (Why: better performance for large documents) - Suggesting to show 100 rows per page
  • Multi fields should no longer have 2 rows, so it would need a different solution (An icon with tooltip for example? Or just adding multi field as bold text to the field name?), reminder, that's how it looks like today:

Bildschirmfoto 2021-06-10 um 22 12 41

FYI: @andreadelrio & @ryankeairns for design input

@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 22, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kertal kertal added the Feature:Discover Discover Application label Dec 22, 2020
@kertal kertal changed the title [Discover] Replace doc viewer table with EuiTable [Discover] Replace doc viewer table with EuiInMemoryTable Jun 10, 2021
@andreadelrio
Copy link
Contributor

One piece of legacy to remove, the doc viewer table using bootstrap of Discover should be replaced with EuiInMemoryTable

Other small thing I noticed in here (and can wait) is that we really need to replace the doc viewer table that shows in the expanded flyout to use an EuiTable. Right now it's using Bootstrap, and we're looking to remove that next minor.

Originally posted by @snide in #67259 (comment)

Here's the very quick mocking code for this, adapted in

https://github.com/elastic/kibana/blob/65251051fdcb948bfb0bc03ad211b851bdb877bd/src/plugins/discover/public/application/components/table/table.tsx

const usedColumns = [
    {
      field: 'action',
      name: 'Action',
      width: 100,
      sortable: false,
      render: () => (
        <>
          <EuiButtonIcon
            aria-label={i18n.translate('discover.docViews.table.filterForValueButtonAriaLabel', {
              defaultMessage: 'Filter for value',
            })}
            className="kbnDocViewer__actionButton"
            data-test-subj="addInclusiveFilterButton"
            iconType={'plusInCircle'}
            iconSize={'s'}
          />
          <DocViewTableRowBtnFilterRemove disabled={false} onClick={() => void 0} />
          <DocViewTableRowBtnToggleColumn active={true} onClick={() => void 0} />
          <DocViewTableRowBtnFilterExists disabled={false} onClick={() => void 0} />
        </>
      ),
    },
    {
      field: 'field',
      name: 'Field',
      sortable: true,
      truncateText: false,
      render: (field) => <FieldName fieldName={field} fieldType={'string'} />,
    },
    {
      field: 'value',
      name: 'Value',
    },
  ];

  return (
    <EuiInMemoryTable
      items={table}
      columns={usedColumns}
      pagination={true}
    />
  );

Looks like this:

Bildschirmfoto 2021-06-10 um 21 58 55

Requirements

  • It should work like the legacy table in terms of functionality, no truncation of values
  • Only pagination should be added (Why: better performance for large documents) - Suggesting to show 100 rows per page
  • Multi fields should no longer have 2 rows, so it would need a different solution (An icon with tooltip for example? Or just adding multi field as bold text to the field name?), reminder, that's how it looks like today:
Bildschirmfoto 2021-06-10 um 22 12 41

FYI: @andreadelrio & @ryankeairns for design input

@kertal I can provide some mocks for this. I can't remember... did we consider the EuiBadge+tooltip combo to label multifieds?

@dimaanj dimaanj self-assigned this Jun 14, 2021
@kertal
Copy link
Member Author

kertal commented Jun 14, 2021

@kertal I can provide some mocks for this. I can't remember... did we consider the EuiBadge+tooltip combo to label multifieds?

@andreadelrio well, don't think we considered it. think an badge or icon with explanations would be fine 👍

@andreadelrio
Copy link
Contributor

Screen Recording 2021-06-14 at 12 00 14 PM

Attaching a mock for this. How do you all feel about moving the Actions columns to the end? Many apps show actions on hover on the right (e.g. Gmail when hovering emails on your inbox).

@kertal what content would make sense for the tooltip?

@kertal
Copy link
Member Author

kertal commented Jun 16, 2021

Screen Recording 2021-06-14 at 12 00 14 PM

Attaching a mock for this. How do you all feel about moving the Actions columns to the end? Many apps show actions on hover on the right (e.g. Gmail when hovering emails on your inbox).

@kertal what content would make sense for the tooltip?

@andreadelrio thx, sorry for the late reply, overseen the notification. For the tooltip I'm adding @gchaps @shaunmcgough. so how could we explain multi fields in a sentence?

@shaunmcgough
Copy link

I agree with this design, it's elegant and relevant. My one question is in regards to the tooltip. Does it popup automatically, or do you have to click 'multifield'. I would prefer you to have to click it. The text should be relatively easy for @gchaps and I. Perhaps something like "Multifields can have multiple values per field".

@kertal
Copy link
Member Author

kertal commented Jun 16, 2021 via email

@andreadelrio
Copy link
Contributor

I agree with this design, it's elegant and relevant. My one question is in regards to the tooltip. Does it popup automatically, or do you have to click 'multifield'. I would prefer you to have to click it. The text should be relatively easy for @gchaps and I. Perhaps something like "Multifields can have multiple values per field".

I would recommend that the tooltip appears on hover which is the usual behavior. Badges can have onClick actions but displaying a tooltip is not usually one of them.

@gchaps
Copy link
Contributor

gchaps commented Jun 17, 2021

We use "multi-field" with a hyphen, so we should use that in both the text and the badge.

Multi-fields can have multiple values per field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants