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

Feature/data explorer surroundings #4740

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Aug 14, 2023

Description

Separate context application and discover state. Discover application now has two applications states:

export interface DiscoverRootState extends RootState {
  discover: DiscoverState;
  discoverContext: DiscoverContextState;
}

Allow view to register multiple redux slices

Slices is now an Slice array and views can register multiple slices

readonly ui?: {
    defaults: T | (() => T) | (() => Promise<T>);
    slices: Slice[];
  };

views.forEach((view) => {
    if (!view.ui) return;

    view.ui.slices.forEach((slice) => {
      registerSlice(slice);
    });
  });

For defaults, modify the logic. To minimize the impact, check if (view.id in defaultResult) {. Previously, Discover only has one slice discover. If an application wants to have multiple slices, one slice should be named as view.id. For example, if Discover want to register multiple slices, one slice should be named as discover. This would make sure that the change won't affect current usage and provide us a simple check clause.

if (typeof defaults === 'function') {
      const defaultResult = await defaults();

      // Check if the result contains the view's ID key.
      // This is used to distinguish between a single registered state and multiple states.
      // Multiple registered states should return an object with multiple key-value pairs with one key always equal to view.id.
      if (view.id in defaultResult) {
        for (const key in defaultResult) {
          // The body of a for-in should be wrapped in an if statement to filter unwanted properties from the prototype
          if (defaultResult.hasOwnProperty(key)) {
            rootState[key] = defaultResult[key];
          }
        }
      } else {
        rootState[view.id] = defaultResult;
      }

Add surrounding flyout SurrondingDocumentFlyout. This component will be render if there is an clicked document (expandedHit) and surroundingFlyoutOpen is true. This surroundingFlyoutOpen is controlled by clicking View Surrouding documents.

Restore original context application to surrounding flyout

Restore and simplify the fetch logic from context application (use_query_actions.ts)

Issues Resolved

#4231
#4230

Screenshot

Screenshot 2023-08-14 at 15 30 46

ToDo

  1. Toggle doesn’t update the surrounding document
  2. Update surrounding document is slow and should settle at the same time
  3. Format table
  4. Implement and test resize.

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

In this PR:
* separate context application and discover state.
* allow view to register multiple redux slices
* add surrounding flyout
* restore original context application to surrounding flyout
* restore and simplify the fetch logic from context application

Issue Resolve
opensearch-project#4231
opensearch-project#4230

Signed-off-by: ananzh <ananzh@amazon.com>
@ananzh ananzh force-pushed the feature/data-explorer-surroundings branch from 9b38cc3 to f55f221 Compare August 14, 2023 23:07
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #4740 (68701b1) into feature/data-explorer (521f306) will decrease coverage by 0.26%.
Report is 3 commits behind head on feature/data-explorer.
The diff coverage is 57.58%.

@@                    Coverage Diff                    @@
##           feature/data-explorer    #4740      +/-   ##
=========================================================
- Coverage                  66.50%   66.25%   -0.26%     
=========================================================
  Files                       3289     3385      +96     
  Lines                      62821    64727    +1906     
  Branches                    9788    10315     +527     
=========================================================
+ Hits                       41782    42885    +1103     
- Misses                     18658    19307     +649     
- Partials                    2381     2535     +154     
Flag Coverage Δ
Linux_2 55.04% <84.33%> (?)
Linux_4 35.10% <14.56%> (?)
Windows ?
Windows_1 34.84% <8.96%> (?)
Windows_2 55.01% <84.33%> (?)
Windows_3 43.87% <43.13%> (?)
Windows_4 35.10% <14.56%> (?)

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

Files Changed Coverage Δ
packages/osd-babel-preset/node_preset.js 100.00% <ø> (ø)
...osd-opensearch-archiver/src/lib/docs/test_stubs.ts 68.75% <ø> (ø)
...ch-archiver/src/lib/indices/create_index_stream.ts 69.76% <ø> (ø)
...ch-archiver/src/lib/indices/delete_index_stream.ts 50.00% <ø> (ø)
...ver/src/lib/indices/opensearch_dashboards_index.ts 0.00% <0.00%> (ø)
packages/osd-ui-shared-deps/theme.ts 41.66% <0.00%> (ø)
src/core/server/legacy/legacy_service.ts 75.38% <ø> (ø)
src/core/server/plugins/plugin_context.ts 45.45% <ø> (ø)
src/core/server/rendering/views/template.tsx 100.00% <ø> (ø)
...server/saved_objects/saved_objects_service.mock.ts 100.00% <ø> (ø)
... and 71 more

... and 216 files with indirect coverage changes

Signed-off-by: ananzh <ananzh@amazon.com>
@ananzh
Copy link
Member Author

ananzh commented Aug 24, 2023

close this due to design change from flyout to new page
new PR is #4816

@ananzh ananzh closed this Aug 24, 2023
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.

2 participants