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

Use AG Grid instead of the old table.js rendering #5915

Merged
merged 11 commits into from
Mar 15, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Mar 13, 2023

Pull Request Description

image

Expanded view:
image

Truncated view:
image

Replaces the code generating the JS Table visualization with AG Grid Community.

  • Supports all of the inputs from the old table.js support.
  • Consistently renders values regardless of source.
  • No support for nested tables - by design, we can assess value later.
  • Grid allows sorting and filtering.
  • Pins the header row.
  • Alternate row coloring.

Important Notes

To switch to the Enterprise edition, we would need to solve the licensing issues. This would add:

  • Filter with values in the grid (like Excel).
  • Column auto-sizing.
  • Column pining.
  • Clipboard copy support (subject to solving the electron issue) - including with or without headers.
  • Ability to export to Excel and CSV from the grid.
  • Column selector.
  • Better options for larger datasets (community only supports infinite scroll).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@farmaazon
Copy link
Contributor

I hope you haven't spent too much time on it, as the table visualization will be replaced anyway in #5806 with a completely different take on rendering tables. I don't think it's compatible with AG Grid. Surely, we cannot rely on its filtering/sorting, as those need cooperation with the engine.

@jdunkerley
Copy link
Member Author

I hope you haven't spent too much time on it, as the table visualization will be replaced anyway in #5806 with a completely different take on rendering tables. I don't think it's compatible with AG Grid. Surely, we cannot rely on its filtering/sorting, as those need cooperation with the engine.

A small piece of work to get closer to the functionality we need for now. Once the rust-based grids can do everything we need we can swap.

Filtering and sorting indeed is limited to only the cases when we show the whole set.
The server-side data options might enable this on larger sets later.

@jdunkerley jdunkerley requested a review from radeusgd as a code owner March 14, 2023 11:32
@jdunkerley jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 14, 2023
@jdunkerley jdunkerley force-pushed the wip/jd/aggrid-prototype branch from a84b0e5 to db5c93d Compare March 14, 2023 11:34
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

For a long time I was thinking that it may be easiest to visualize objects by their to_text/to_display_text representation - what is the benefit on relying on JSON for the cell contents? So I'm thinking if the new Date handling is needed - IMO we should just rely on our Text representation.

As for the 1-indexed rows - OK, but let's add a note to the documentation of places where there will now be inconsistency.

Looks good overall, cool that we are getting this cool table vis!

Approving, but please make sure we are not displaying a wrong timezone on the date visualization - this is something that I think we must avoid.

@jdunkerley
Copy link
Member Author

Filtering and sorting indeed is limited to only the cases when we show the whole set.

The grid will disable filter/sort if the row set is truncated (1000 rows limit currently). There will also be a pinned row added to tell the user this and the total row count.

@jdunkerley jdunkerley force-pushed the wip/jd/aggrid-prototype branch 2 times, most recently from b517397 to 484d8f1 Compare March 14, 2023 20:43
Comment on lines 4 to 5
loadStyle('https://cdn.jsdelivr.net/npm/ag-grid-community/styles/ag-grid.css')
loadStyle('https://cdn.jsdelivr.net/npm/ag-grid-community/styles/ag-theme-alpine.css')
Copy link
Member

Choose a reason for hiding this comment

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

Just a note here. Nothing to fix here, as we are doing it wrongly in other places as well, but basically this is bad, as it makes the app not working without internet connection. We need to fix this and other similar places one day. CC @sylwiabr

@jdunkerley jdunkerley force-pushed the wip/jd/aggrid-prototype branch from 39a4e40 to 962443d Compare March 15, 2023 08:53
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Mar 15, 2023
@mergify mergify bot merged commit 7394a1a into develop Mar 15, 2023
@mergify mergify bot deleted the wip/jd/aggrid-prototype branch March 15, 2023 11:09
@jdunkerley jdunkerley linked an issue Mar 15, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial prototype to use AG Grid for Data Grid view
4 participants