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

[Table list view] Integrate package into visualizations, maps, graphs #140040

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Sep 5, 2022

In this PR I've integrated the package version of the <TableListView /> component into the following plugins:

  • Visualizations
  • Maps
  • Graphs

There aren't any new functionality added.

How to test

  • Launch the cloud deployment
  • Navigate to the app and make sure the list view renders correctly

Edit

This PR also update the Table list view to the new <EuiPageTemplate /> component

Screenshot 2022-09-08 at 14 59 32

Screenshot 2022-09-08 at 14 59 48

Screenshot 2022-09-08 at 15 00 03

Screenshot 2022-09-08 at 15 00 27

@sebelga sebelga changed the title Integrate package into visualizations plugin [Table list view] Integrate package into visualizations, maps, graphs Sep 5, 2022
@sebelga sebelga self-assigned this Sep 5, 2022
@sebelga sebelga added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Feature:Content Management User generated content (saved objects) management labels Sep 5, 2022
@sebelga sebelga added the ci:cloud-deploy Create or update a Cloud deployment label Sep 6, 2022
@sebelga sebelga marked this pull request as ready for review September 6, 2022 14:14
@sebelga sebelga requested a review from a team September 6, 2022 14:14
@sebelga sebelga requested review from a team as code owners September 6, 2022 14:14
@sebelga sebelga requested a review from rshen91 September 6, 2022 14:15
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Presentation team changes LGTM

@@ -138,8 +162,18 @@ function TableListViewComp<T extends UserContentCommonSchema>({
}),
sortable: true,
render: (field: keyof T, record: T) => (
// The validation is handled at the top of the component
// eslint-disable-next-line @elastic/eui/href-or-on-click
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not having any issues locally without eslint-disable-next-line...? Can you check and see if this can be removed? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's strange, after doing yarn kbn bootstrap did you restart the eslint server in VS Code? (or simply restart VS Code).

Screenshot 2022-09-07 at 08 12 33

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the eslint error that I get

Screenshot 2022-09-07 at 08 11 14

headingId="visualizeListingHeading"
// we allow users to create visualizations even if they can't save them
// for data exploration purposes
createItem={createNewVis}
tableCaption={i18n.translate('visualizations.listing.table.listTitle', {
defaultMessage: 'Visualize Library',
})}
findItems={fetchItems}
deleteItems={visualizeCapabilities.delete ? deleteItems : undefined}
editItem={visualizeCapabilities.save ? editItem : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

More a question than anything

edit.mp4

I'm testing locally and I'm having issues with the edit button with Lens specific visualizations with sample data...let me know what I'm missing thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great catch @rshen91 ! I fixed it in 8287766

@sebelga sebelga requested a review from rshen91 September 7, 2022 08:52
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

If a vis type can't be found, it's not handled gracefully anymore:

Old (warning label and not clickable):
Screenshot 2022-09-07 at 11 38 13

New:
(doesn't show vis type, but still clickable - tries to navigate to "undefined")
Screenshot 2022-09-07 at 11 36 18
Screenshot 2022-09-07 at 11 36 22

You can test this case by changing the name of a vis type definition (for example here):

@sebelga
Copy link
Contributor Author

sebelga commented Sep 8, 2022

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Sep 8, 2022

Thanks for the review @flash1293 ! Good catch for the page reload. I fixed it in 73b6ba9

@sebelga sebelga requested a review from flash1293 September 8, 2022 14:04
@sebelga
Copy link
Contributor Author

sebelga commented Sep 8, 2022

To those subscribed to this PR notifications: I've had to update the TableListView to use the new <EuiPageTemplate /> as #139524 got merged. I updated the description with screenshots on how each app listing renders now.

@sebelga sebelga requested a review from a team as a code owner September 9, 2022 10:42
@flash1293
Copy link
Contributor

The functional test failures here look legit

@sebelga
Copy link
Contributor Author

sebelga commented Sep 13, 2022

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Sep 13, 2022

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Sep 13, 2022

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Sep 13, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 13, 2022

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [8eba3e6]

History

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

cc @sebelga

@sebelga sebelga merged commit b236f9c into elastic:table-list-view/enhance-ux Sep 14, 2022
@sebelga sebelga deleted the table-list-view/integrate-package-vis-map-graph branch September 14, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:cloud-deploy Create or update a Cloud deployment Feature:Content Management User generated content (saved objects) management Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants