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.

Sign up for GitHub

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

Jump to bottom

[Table Visualization] make table vis column sortable #2502

Merged
merged 1 commit into from
Oct 12, 2022
Merged

[Table Visualization] make table vis column sortable #2502

merged 1 commit into from
Oct 12, 2022

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Oct 4, 2022

Description

  • add sort state (asc | desc) to column

Partially resolve:

#2305

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@ananzh ananzh added the tableVis table visualization label Oct 4, 2022
@ananzh ananzh self-assigned this Oct 4, 2022
@ananzh ananzh requested a review from a team as a code owner October 4, 2022 22:13
@ananzh ananzh mentioned this pull request Oct 4, 2022
Closed
joshuarrrr
joshuarrrr reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Nice progress. I don't have all the context of how this should be working, but it seems like the state handling could be simplified a bit more.

src/plugins/vis_type_table/public/components/table_vis_component.tsx Outdated Show resolved Hide resolved
src/plugins/vis_type_table/public/components/table_vis_component.tsx Outdated
Comment on lines 31 to 38
// set sort and resize column state
const [sortColumn, setSortColumn] = useState<SortColumn>(
handlers.uiState.get('vis.sortColumn') || {}
);

const [columnsWidth, setColumnsWidth] = useState<ColumnWidth[]>(
handlers.uiState.get('vis.columnsWidth') || []
);
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need these local states? It seems like you can just use handlers.uiState getters/setters and the curColumnsState.

Copy link
Member Author

@ananzh ananzh Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a similar question to Ashwin's. I post some details there:

uiState.set won’t let me to render the component. If I switch setSortColumn(updatedVisState.sortColumn); to uiState.set than it won’t re-render with corrected sorted columns. I am using an example from charts ([https://github.com/opensearch-project/OpenSearch-Dashboards/blob/d7004dc5b0392477f[…]3b/src/plugins/charts/public/static/components/color_schema.tsx](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/d7004dc5b0392477fdd54ac66b29d231975a173b/src/plugins/charts/public/static/components/color_schema.tsx#L64) ) to force it work. Regarding this part, I could open an issue as a post research topic. Currently, I couldn’t find anything meaningful now.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed local states

src/plugins/vis_type_table/public/components/table_vis_component.tsx Show resolved Hide resolved
ashwin-pc
ashwin-pc reviewed Oct 5, 2022
View reviewed changes
src/plugins/vis_type_table/public/components/table_vis_component.tsx Outdated

const pagination = usePagination(visConfig, rows.length);

// set sort and resize column state
const [sortColumn, setSortColumn] = useState<SortColumn>(
handlers.uiState.get('vis.sortColumn') || {}
Copy link
Member

@ashwin-pc ashwin-pc Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From what i know about UIState, its a way to get and set state information inside a component rendered by an expression function. We should not be maintaining duplicate state here. If we want to set the new sortColumn value, just use the uiState.set function. The Visualization should rerender with the updated value automatically.

If for some reason it does not update, we need to look into that. But using the useState to trigger the rerender is a hacky workaround

Copy link
Member Author

@ananzh ananzh Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @ashwin-pc, regarding this feedback (#2502 (comment) ), you are right, uiState.set won’t let me to render the component. If I switch setSortColumn(updatedVisState.sortColumn); to uiState.set than it won’t re-render with corrected sorted columns. I am using an example from charts (https://github.com/opensearch-project/OpenSearch-Dashboards/blob/d7004dc5b0392477f[…]3b/src/plugins/charts/public/static/components/color_schema.tsx ) to force it work. Regarding this part, I could open an issue as a post research topic. Currently, I couldn’t find anything meaningful now.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed local state.

This was referenced Oct 11, 2022
Closed
Closed
@ananzh
[Table Visualization] make table vis column sortable
65181fb 
* add sort state (asc | desc)  to column

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh merged commit 7e969bc into opensearch-project:feature/tableVis Oct 12, 2022
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Oct 25, 2022
@ananzh
[Table Visualization] remove legacy table and add table visualization (…
bde624f 
…opensearch-project#2279)

To convert the table visualization into React & OUI DataGrid component,
in this PR, we did two main things:
* clean out legacy angular code
* restore table vis in react
* Datagrid component does not support splitted grids. For future transfer
to OUI Datagrid, we create a tableGroup in visData for splitted grids.

issue resolved:
opensearch-project#2212
https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2213a

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

rename visTable back to opensearch_dashboards_table and add stronger type

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] add a plain datagrid component (opensearch-project#2390)

implement a plain OuiDataGrid component use the basic
pagenation, sort and format.

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] restore datagrid columns (opensearch-project#2411)

* restore datagrid columns
* display column title correctly
* deangular and re-use formatted column
* convert formatted column to data grid column
* restore filter in and filter out value functions

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] restore pagination to table vis (opensearch-project#2461)

* add pagination

patically resolved:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] make table vis column resizable (opensearch-project#2464)

* add resizable state to column

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] make table vis column sortable (opensearch-project#2502)

* add sort state (asc | desc)  to column
* fix pagination issue

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] format table cell and restore showTotal feature (opensearch-project#2562)

* format table cell to show Date and percent
* restore showTotal feature: it allows table vis to show total,
avg, min, max and count statics on count
* fix some type errors

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization]restore export csv feature to table vis (opensearch-project#2568)

* add addtional action in toolbar to allow export data to csv.
there are two types of csv, raw and formatted. raw is the original
data and formatted is to show formatted Date and percentage data
when needed.
* when table is not saved, export csv file will be named as
unsaved-raw.csv if choose raw. when table is saved with a filename,
it will be saved as [filename]-[raw/formatted].csv

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] split table in rows and columns

This PR implement a group component TableVisGroupComponent utilizing
TableVisComponent as sub component. It also adds a title to TableVisComponent.

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] remove local state

* remove local col width state to allow split tables to fetch
updated col width state
* fix type errors in usePagination

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] remove repeated column from split tables

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization][BUG] partical rows option generate repeated metrics

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Oct 28, 2022
@ananzh
[Table Visualization] remove legacy table and add table visualization (…
b39a865 
…opensearch-project#2279)

To convert the table visualization into React & OUI DataGrid component,
in this PR, we did two main things:
* clean out legacy angular code
* restore table vis in react
* Datagrid component does not support splitted grids. For future transfer
to OUI Datagrid, we create a tableGroup in visData for splitted grids.

issue resolved:
opensearch-project#2212
https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2213a

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

rename visTable back to opensearch_dashboards_table and add stronger type

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] add a plain datagrid component (opensearch-project#2390)

implement a plain OuiDataGrid component use the basic
pagenation, sort and format.

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] restore datagrid columns (opensearch-project#2411)

* restore datagrid columns
* display column title correctly
* deangular and re-use formatted column
* convert formatted column to data grid column
* restore filter in and filter out value functions

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] restore pagination to table vis (opensearch-project#2461)

* add pagination

patically resolved:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] make table vis column resizable (opensearch-project#2464)

* add resizable state to column

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] make table vis column sortable (opensearch-project#2502)

* add sort state (asc | desc)  to column
* fix pagination issue

Partially resolve:
opensearch-project#2305

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] format table cell and restore showTotal feature (opensearch-project#2562)

* format table cell to show Date and percent
* restore showTotal feature: it allows table vis to show total,
avg, min, max and count statics on count
* fix some type errors

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization]restore export csv feature to table vis (opensearch-project#2568)

* add addtional action in toolbar to allow export data to csv.
there are two types of csv, raw and formatted. raw is the original
data and formatted is to show formatted Date and percentage data
when needed.
* when table is not saved, export csv file will be named as
unsaved-raw.csv if choose raw. when table is saved with a filename,
it will be saved as [filename]-[raw/formatted].csv

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization] split table in rows and columns (opensearch-project#2578)

This PR implement a group component TableVisGroupComponent utilizing
TableVisComponent as sub component. It also adds a title to TableVisComponent.

Partically resolved:
opensearch-project#2379

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization][IMPROVE] remove repeated column from split tables (opensearch-project#2583)

Currently, when we split table by columns, the split column is shown
both in the table title and as a separate column. This is not needed.
In this PR, we remove the repeated column in split tables in col.

Partically resolved:
opensearch-project#2579 (comment)

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization][BUG] remove local column width state (opensearch-project#2582)

* remove local col width state to allow split tables to fetch
updated col width state
* fix type errors in usePagination

Partially resolved:
opensearch-project#2579 (comment)

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

[Table Visualization][BUG] partical rows shows metrics for all columns (opensearch-project#2648)

Currently, when we enable Show partial rows in the Options panel, we see metrics been
added to every column even though Show metrics for every bucket/level is not enabled.

Metrics are added and returned when we enable the partial rows. This PR fixed the bug
by slice the returned data to allow only the last set of metrices.

Partially resolved:
opensearch-project#2579 (comment)

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuarrrr joshuarrrr joshuarrrr left review comments

@ashwin-pc ashwin-pc ashwin-pc left review comments

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki

Assignees

@ananzh ananzh

Labels
tableVis table visualization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ananzh @joshuarrrr @ashwin-pc