Skip to content

Create a widget to query ECSQL strings#213

Draft
achrysaetos wants to merge 8 commits into
masterfrom
leck/ecsql-widget
Draft

Create a widget to query ECSQL strings#213
achrysaetos wants to merge 8 commits into
masterfrom
leck/ecsql-widget

Conversation

@achrysaetos
Copy link
Copy Markdown
Contributor

@achrysaetos achrysaetos commented Aug 15, 2022

On startup:
image

Bad query:
image

Successful query:
image

Comment thread common/changes/@itwin/viewer-react/leck-ecsql-widget_2022-08-15-20-49.json Outdated
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx Outdated
@aruniverse
Copy link
Copy Markdown
Member

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

You probably want to be using a Virtualized Table

Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx Outdated
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx Outdated
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx Outdated
.forEach((key) => {
row[key] = JSON.stringify(obj[key]);
});
// Case 3: If the cell content is too long, put it inside an ExpandableBlock
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx Outdated
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx
Comment thread packages/modules/viewer-react/src/components/app-ui/EcsqlWidget.tsx
public readonly id = "EcsqlWidgetProvider";

public provideWidgets(
stageId: string,
Copy link
Copy Markdown
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
Copy Markdown
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...

Comment thread packages/apps/web-viewer-test/src/providers/EcsqlWidget.tsx Outdated
Copy link
Copy Markdown
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 ;)

Comment thread packages/apps/web-viewer-test/src/providers/EcsqlWidget.tsx

rows.push(row);
for (const key of Object.keys(row)) {
if (!columnHeaders.includes(key)) {
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread packages/apps/web-viewer-test/src/providers/EcsqlWidget.tsx
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