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

Create a widget to query ECSQL strings #213

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

achrysaetos
Copy link
Contributor

@achrysaetos achrysaetos commented Aug 15, 2022

On startup:
image

Bad query:
image

Successful query:
image

@aruniverse
Copy link
Member

  1. the table becomes very slow if the number of results exceeds 5000 rows

You probably want to be using a Virtualized Table

.forEach((key) => {
row[key] = JSON.stringify(obj[key]);
});
// Case 3: If the cell content is too long, put it inside an ExpandableBlock
Copy link
Member

Choose a reason for hiding this comment

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

what does this actually look like?

Copy link
Member

Choose a reason for hiding this comment

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

what is the type for row here when this occurs? i dont think it matches what you had originally typed it as

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this:
image

When expanded:
image

Copy link
Member

Choose a reason for hiding this comment

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

still feels a little weird for me, have you looked at what the standard pattern we use is?
do we just show as much text as we can before showing an ellipses for overflow, and then on hover show the full text in a tool tip or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iModelConsole shows as much as it can in a cell before showing an ellipses for overflow, and then clicking the cell expands the whole row to display the full text. The Table from core doesn't have anything like that, I think, there's just a button to expand the whole row
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe I just have to customize it a bit more.

public readonly id = "EcsqlWidgetProvider";

public provideWidgets(
stageId: string,
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at what we do for stageId and stageUsage in the other providers in viewer-components-react

@achrysaetos
Copy link
Contributor Author

  1. the table becomes very slow if the number of results exceeds 5000 rows

You probably want to be using a Virtualized Table

Unfortunately, the virtualized table loses the horizontal scrollbar if there are too many columns, and it can't be added back without some (hacky?) css ☹️. Also for some long strings, the virtualized table will only write the visible portion to the DOM, which results in it trying to constantly format each string to fit inside its cell, leading to a jittery experience imo.

The performance gain is very important though...

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

Good work! We'll take it over once you go back to school. Unless you want to continue contributing as an OpenSource Collaborator ;)


rows.push(row);
for (const key of Object.keys(row)) {
if (!columnHeaders.includes(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

well wouldnt they always all not be in columnheaders to begin with?
re-read this block of code here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm pretty sure this is right. I wanted columnHeaders to be the superset of all keys in each row, because sometimes each row might have different keys. So every time columnHeaders encounters a new unique key, it should append the new key. columnHeaders is instantiated before we start iterating through each row.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants