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

Add Drag & Drop Across Axis Functionality to Vis Builder #7107

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LDrago27
Copy link
Collaborator

@LDrago27 LDrago27 commented Jun 25, 2024

Description

This change aims to enhance the drag and drop functionality present within Vis-Builder. The enhancements are

  • Users can drag a new field from left panel and drop it over an existing field (replacing the existing field)
  • User can move a selected field from one axis to an empty axis
  • User can move a selected field from one axis to another axis replacing an existing field.

To support the Drag and Drop functionality the custom Drag and Drop containers were converted to Draggable and Droppable Components (part of OUI). Besides the OUIAccordion didn't support Dragging of the elements present within the accordion hence instead of using it a custom version of Accordion was used.

Issues Resolved

#7015

Screenshot

EditedEnhancementVisBUilder.mp4

Testing the changes

Changelog

  • feat: Enhance Drag & Drop functionality in Vis Builder

Check List

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

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

opensearch-changeset-bot bot added a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 25, 2024
opensearch-changeset-bot bot added a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 25, 2024
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 12.40876% with 120 lines in your changes missing coverage. Please review.

Project coverage is 67.37%. Comparing base (e74ed2c) to head (34ab28f).
Report is 18 commits behind head on main.

Files Patch % Lines
...r/public/application/components/data_tab/index.tsx 0.00% 80 Missing ⚠️
...mponents/data_tab/utils/get_valid_aggregations.tsx 21.73% 18 Missing ⚠️
...pplication/components/data_tab/use/use_dropbox.tsx 0.00% 11 Missing ⚠️
...public/application/components/data_tab/dropbox.tsx 0.00% 9 Missing ⚠️
...c/application/components/data_tab/config_panel.tsx 0.00% 1 Missing ⚠️
...lication/components/data_tab/schema_to_dropbox.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7107      +/-   ##
==========================================
- Coverage   67.45%   67.37%   -0.08%     
==========================================
  Files        3442     3450       +8     
  Lines       67816    68057     +241     
  Branches    11027    11086      +59     
==========================================
+ Hits        45742    45853     +111     
- Misses      19408    19531     +123     
- Partials     2666     2673       +7     
Flag Coverage Δ
Linux_1 33.05% <12.40%> (-0.03%) ⬇️
Linux_2 55.06% <ø> (-0.06%) ⬇️
Linux_3 45.25% <ø> (+0.05%) ⬆️
Linux_4 34.83% <ø> (-0.04%) ⬇️
Windows_1 33.07% <12.40%> (-0.03%) ⬇️
Windows_2 55.01% <ø> (-0.06%) ⬇️
Windows_3 45.25% <ø> (+0.04%) ⬆️
Windows_4 34.83% <ø> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LDrago27 LDrago27 force-pushed the pocDragDropMigration branch 2 times, most recently from 6e1483e to 56e4202 Compare June 26, 2024 00:03
@LDrago27 LDrago27 changed the title Add Drag Across Axis Functionality to Vis Builder Add Drag & Drop Across Axis Functionality to Vis Builder Jun 26, 2024
opensearch-changeset-bot bot added a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 26, 2024
@LDrago27 LDrago27 force-pushed the pocDragDropMigration branch 5 times, most recently from 4b31db9 to bd08a03 Compare June 27, 2024 17:53
@LDrago27 LDrago27 marked this pull request as ready for review June 27, 2024 18:38

export const DATA_TAB_ID = 'data_tab';

const getIndexPatternField = (indexFieldName: string, availableFields: IndexPatternField[]) =>
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate of

const getIndexPatternField = (indexFieldName: string, availableFields: IndexPatternField[]) =>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have consolidated the common functions into a util and reused it across required components.

})
.filter((aggType) => (isCountField ? true : aggType.name !== 'count'))
// `types` can be either a Bucket or Metric aggType, but both types have the name property.
.map((aggType) => (aggType as BucketAggType).name)
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get this part. If types can be both metric or bucket agg, why we assign only bucket agg type here aggType as BucketAggType?

}
};

const handleConfigurationPanelTransition = ({
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we memorize this computation func with useCallback? same question for handleFieldSelectorToConfigurationPanelTransition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion that might not work here. The params that we have are source, destination, combine which have droppableId of the area and an index that to the fieldAggregation i.e being moved around. So ideally for 2 differrent transitions the index value may remain same but the underlying fieldAggreagtion can be different. In those scenarios the memoization can prove to be detrimental.

ananzh
ananzh previously approved these changes Jul 1, 2024
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.

None yet

2 participants