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 view with two way sync #1022

Merged

Conversation

abasatwar
Copy link
Contributor

@abasatwar abasatwar commented Sep 14, 2022

Table view visualization with two way sync is implemented.
#916

@abasatwar abasatwar requested a review from a team as a code owner September 14, 2022 08:48
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Merging #1022 (3403ad7) into main (bdf5291) will increase coverage by 0.01%.
The diff coverage is 57.69%.

@@             Coverage Diff              @@
##               main    #1022      +/-   ##
============================================
+ Coverage     53.60%   53.61%   +0.01%     
  Complexity      291      291              
============================================
  Files           280      280              
  Lines          9309     9335      +26     
  Branches       2135     2155      +20     
============================================
+ Hits           4990     5005      +15     
- Misses         4149     4160      +11     
  Partials        170      170              
Flag Coverage Δ
dashboards-observability 47.60% <57.69%> (+0.03%) ⬆️
opensearch-observability 71.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ashboards-observability/common/constants/shared.ts 100.00% <ø> (ø)
...ts/visualizations/charts/data_table/data_table.tsx 46.06% <54.16%> (+2.99%) ⬆️
...isualizations/charts/data_table/data_table_type.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@abasatwar abasatwar marked this pull request as draft September 14, 2022 14:53
@abasatwar abasatwar marked this pull request as ready for review September 19, 2022 11:02
spattnaik
spattnaik previously approved these changes Sep 19, 2022
mengweieric
mengweieric previously approved these changes Sep 19, 2022
@@ -46,6 +33,30 @@ export const DataTable = ({ visualizations, layout, config }: any) => {
metadata: { fields = [] },
} = visualizations.data.rawVizData;

const { dataConfig = {} } = visualizations?.data?.userConfigs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we follow bar's viz data destruction code here to destruct data config so we don't have to make these multiple long lines of assignments?

typeof dataConfig?.chartStyles?.columnWidth !== 'undefined'
? dataConfig?.chartStyles?.columnWidth
: visualizations.vis.columnwidth;

useEffect(() => {
document.addEventListener('keydown', hideGridFullScreenHandler);
return () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

from Line 70, seems like we can also use useMemo hook here, could we add this for column calculation?

@mengweieric
Copy link
Collaborator

Please address the comments and resolve the conflicts for this PR.

@ruchika-narang ruchika-narang force-pushed the feature/table-visualization branch from 4f15c16 to 03dc15e Compare September 30, 2022 06:31
abasatwar and others added 5 commits September 30, 2022 06:53
Signed-off-by: abasatwar <[email protected]>
Signed-off-by: abasatwar <[email protected]>
Signed-off-by: ruchika-narang <[email protected]>
@ruchika-narang ruchika-narang force-pushed the feature/table-visualization branch from 21ae219 to 813e24f Compare September 30, 2022 06:53
@mengweieric mengweieric merged commit 640462f into opensearch-project:main Sep 30, 2022
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.

7 participants