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

[D&D] metadata slice 1879 #2193

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

abbyhu2000
Copy link
Member

@abbyhu2000 abbyhu2000 commented Aug 23, 2022

Description

Add two new state in metadata slice, disable save button when the visualization is not savable and display error messages using tooltip.

Visualization is not savable when:

  1. Empty visualization
  2. No new changes detected after the visualization is loaded
  3. Has unapplied/unsaved aggregation changes
  4. Immediately after clicking the save button

Issues Resolved

resolves #1879

Check List

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

@abbyhu2000 abbyhu2000 self-assigned this Aug 23, 2022
@abbyhu2000 abbyhu2000 force-pushed the brb_metadata_slice_1879 branch from 80f3f24 to 2a25918 Compare August 24, 2022 00:18
@abbyhu2000 abbyhu2000 changed the title Brb metadata slice 1879 [D&D] metadata slice 1879 Aug 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022

Codecov Report

Merging #2193 (6a436d9) into main (730a75a) will decrease coverage by 0.01%.
The diff coverage is 5.55%.

@@            Coverage Diff             @@
##             main    #2193      +/-   ##
==========================================
- Coverage   67.22%   67.21%   -0.02%     
==========================================
  Files        3100     3100              
  Lines       59564    59579      +15     
  Branches     9062     9064       +2     
==========================================
+ Hits        40043    40047       +4     
- Misses      17334    17346      +12     
+ Partials     2187     2186       -1     
Impacted Files Coverage Δ
...public/application/utils/state_management/store.ts 15.00% <0.00%> (-35.00%) ⬇️
...plication/utils/state_management/metadata_slice.ts 45.45% <33.33%> (-4.55%) ⬇️
packages/osd-optimizer/src/node/cache.ts 52.77% <0.00%> (+2.77%) ⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 87.75% <0.00%> (+4.08%) ⬆️

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

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Nice change. Almost all my comments are stylistic

…sable the save button if not and show error messages.

Signed-off-by: abbyhu2000 <[email protected]>
@abbyhu2000 abbyhu2000 force-pushed the brb_metadata_slice_1879 branch from 2a25918 to 3c1da32 Compare August 24, 2022 17:03
@abbyhu2000 abbyhu2000 marked this pull request as ready for review August 24, 2022 17:08
@abbyhu2000 abbyhu2000 requested a review from a team as a code owner August 24, 2022 17:08
ashwin-pc
ashwin-pc previously approved these changes Aug 25, 2022
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Looks great!

import { useTypedSelector, useTypedDispatch } from '../utils/state_management';
import { setSaveState } from '../utils/state_management/metadata_slice';

// TODO: Need to finalize the error messages
Copy link
Member

Choose a reason for hiding this comment

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

do we have a follow-up issue for this? do we need ux insight here?

yarn.lock Show resolved Hide resolved

// TODO: Need to finalize the error messages
if (isEmpty) {
errorMsg = 'The canvas is empty. Add some aggregations before saving.';
Copy link
Member

Choose a reason for hiding this comment

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

do we need to ping @KrooshalUX to get the final messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

@KrooshalUX Hi Kroosh, for the save button tooltip() error messages for wizard, have we finalized them? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@btzeng - do we have an empty state for this view?

@ananzh
Copy link
Member

ananzh commented Aug 25, 2022

cool~ ship it!

joshuarrrr
joshuarrrr previously approved these changes Aug 25, 2022
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.

Awesome job on the feature!

@abbyhu2000 abbyhu2000 dismissed stale reviews from joshuarrrr, kavilla, and ashwin-pc via 20df493 August 25, 2022 19:00
@abbyhu2000 abbyhu2000 force-pushed the brb_metadata_slice_1879 branch from 202acd0 to 20df493 Compare August 25, 2022 19:00
@abbyhu2000 abbyhu2000 force-pushed the brb_metadata_slice_1879 branch from 20df493 to 9f17933 Compare August 25, 2022 19:01
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Ship it!

@kavilla kavilla added the v2.3.0 label Aug 29, 2022
@ashwin-pc ashwin-pc merged commit 65005be into opensearch-project:main Aug 30, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 30, 2022
* Use metadata slice to save if the visualization is savable or not. Disable the save button if not and show error messages.

Signed-off-by: abbyhu2000 <[email protected]>

* Add some comments about store subscriber and editor state

Signed-off-by: abbyhu2000 <[email protected]>

* create a seperate use_can_save hook

Signed-off-by: abbyhu2000 <[email protected]>

Signed-off-by: abbyhu2000 <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 65005be)
kavilla pushed a commit that referenced this pull request Aug 31, 2022
* Use metadata slice to save if the visualization is savable or not. Disable the save button if not and show error messages.

Signed-off-by: abbyhu2000 <[email protected]>

* Add some comments about store subscriber and editor state

Signed-off-by: abbyhu2000 <[email protected]>

* create a seperate use_can_save hook

Signed-off-by: abbyhu2000 <[email protected]>

Signed-off-by: abbyhu2000 <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 65005be)

Co-authored-by: Qingyang(Abby) Hu <[email protected]>
@abbyhu2000 abbyhu2000 deleted the brb_metadata_slice_1879 branch June 30, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[D&D] Make new "meta" state slice to keep track of things we don't want to save and load
8 participants