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

enhance grouping for context menus #3169

Closed

Conversation

sikhote
Copy link
Contributor

@sikhote sikhote commented Jan 2, 2023

Note: Commits will be signed off as final step.

Description

  • Lowers the minimum children count to 2, allowing for smaller groups within context menus
  • Adds a separator between groups
  • Add documentation and some variable renaming for clarity
  • Adds an option to the embedded panel component to remove borders and padding

Issues Resolved

#3171

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

@sikhote sikhote requested a review from a team as a code owner January 2, 2023 00:03
@sikhote sikhote marked this pull request as draft January 2, 2023 22:38
@sikhote sikhote marked this pull request as ready for review March 12, 2023 23:48
@ashwin-pc ashwin-pc self-assigned this Mar 13, 2023
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.

The main change seems fine. Have some questions around the other changes that arent directly related to the PR title. Also since you dabbled quite deep into UI actions and context menus, can you add documentation to the UI Actions readme too? Currently it does not discuss anything about your change or why its needed. Lets make sure that the next person using context menus and UI actions does not have to do the deep dive you did.

Comment on lines 1 to 4
opensearch.hosts: ["https://localhost:9200"]
opensearch.username: "admin" # Default username on the docker image
opensearch.password: "admin" # Default password on the docker image
opensearch.ssl.verificationMode: none
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this from the PR. We should not be making changes to the YML unless necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed for final cleanup.

package.json Outdated
@@ -11,7 +11,7 @@
"dashboarding"
],
"private": true,
"version": "3.0.0",
"version": "2.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed for final cleanup.

@@ -81,7 +81,7 @@ export interface EmbeddableFactory<
* Returns a display name for this type of embeddable. Used in "Create new... " options
* in the add panel for containers.
*/
getDisplayName(): string;
getDisplayName(): JSX.Element | string;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getDisplayName changes are all related to allowing the names to also accept JSX, rather than just strings. In some situations we can now pass icons into the title, enhancing functionality.

Comment on lines 86 to 91
isRetained?: boolean;
isBorderless?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Did we agree on this change? The source issue only talks about context menus. This changes how the embeddable can be used and the API of embeddables. I would not club this change with this PR. Can we open a separate issue to decide on it first and then make the change?

FYI, i'm not against the feature, but rather, I want to document the new API better so that others are aware of it. e.g. I have no idea what isRetained signifies unless i see this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not spoken with anyone about these changes. The isBorderless prop is helpful in order to display the embeddable panel without any extra styling, which is not desired in our upcoming feature. The isRetained prop prevents the embeddable from being destroyed—I will rename it to isDestroyPrevented, which might be a better name—my goal with the naming is to allow the default to be false. I will also add a comment above the prop for explanation.

As for grouping these changes together—for our feature, all of these changes are needed together. I generally find it ok to group changes, but please let me know if it is required to split them into multiple PRs.

@@ -92,7 +92,7 @@ export interface Action<Context extends BaseContext = {}, T = ActionType>
* Returns a title to be displayed to the user.
* @param context
*/
getDisplayName(context: ActionExecutionContext<Context>): string;
getDisplayName(context: ActionExecutionContext<Context>): JSX.Element | string;
Copy link
Member

Choose a reason for hiding this comment

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

Again, why is this not a string anymore?

Comment on lines +160 to +204

// Reference to the last group...groups are provided in array order of
// parent to children (or outer to inner)
let parentPanelId = '';

if (action.grouping) {
for (let i = 0; i < action.grouping.length; i++) {
const group = action.grouping[i];
currentPanel = group.id;
if (!panels[currentPanel]) {
const groupId = group.id;

if (!panels[groupId]) {
const name = group.getDisplayName ? group.getDisplayName(context) : group.id;
panels[currentPanel] = {
id: currentPanel,

// Create panel for group
panels[groupId] = {
id: groupId,
title: name,
items: [],
_level: i,
_icon: group.getIconType ? group.getIconType(context) : 'empty',
};
if (parentPanel) {
panels[parentPanel].items!.push({

// If there are multiple groups and this is not the first group,
// then add an item to the parent panel relating to this group
if (parentPanelId) {
panels[parentPanelId].items!.push({
name,
panel: currentPanel,
panel: groupId,
icon: group.getIconType ? group.getIconType(context) : 'empty',
_order: group.order || 0,
_title: group.getDisplayName ? group.getDisplayName(context) : '',
});
}
}
parentPanel = currentPanel;

// Save the current panel, because this will be used for adding items
// to it from later groups in the array
parentPanelId = groupId;
}
}
panels[parentPanel || 'mainMenu'].items!.push({

// If grouping exists, parentPanelId will be the most-inner group,
// otherwise use the mainMenu panel.
// Add item for action to this most-inner panel or mainMenu.
panels[parentPanelId || 'mainMenu'].items!.push({
Copy link
Member

Choose a reason for hiding this comment

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

Did anything change here functionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall, for this file, yes. We now have borders separating groups, which was a TODO before. There is also some rewrite to make things more readable and with comments to document what is happening, since this functionality is a bit difficult to follow, even with the comments.

@@ -65,7 +65,7 @@ export interface TriggerContextMapping {
[VISUALIZE_GEO_FIELD_TRIGGER]: VisualizeFieldContext;
}

const DEFAULT_ACTION = '';
export const DEFAULT_ACTION = '';
Copy link
Member

Choose a reason for hiding this comment

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

No issue with this export, just curious why it was needed since you arent using it anywhere in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as it's no longer needed—when i first started working with the actions I did not know how to add in types for new actions, but I figured that out since 👍

Comment on lines +231 to +241
// If the panel is a root-level panel, such as a group-based panel,
// then create mainMenu item for this panel
if (panel._level === 0) {
// TODO: Add separator line here once it is available in EUI.
// See https://github.com/elastic/eui/pull/4018
if (panel.items.length > 3) {
// Add separator with unique key if needed
if (panels.mainMenu.items.length) {
panels.mainMenu.items.push({ isSeparator: true, key: `${panel.id}separator` });
}

// If a panel has more than one child, then allow item's to be grouped
// and link to it in the mainMenu. Otherwise, flatten the group.
if (panel.items.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will add this soon and comment here when completed 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added ✅

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2023

Codecov Report

Merging #3169 (129af78) into main (99a88e3) will decrease coverage by 0.03%.
The diff coverage is 76.92%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3169      +/-   ##
==========================================
- Coverage   66.47%   66.45%   -0.03%     
==========================================
  Files        3209     3210       +1     
  Lines       61617    61679      +62     
  Branches     9504     9524      +20     
==========================================
+ Hits        40961    40989      +28     
- Misses      18384    18404      +20     
- Partials     2272     2286      +14     
Flag Coverage Δ
Linux 66.40% <76.92%> (-0.03%) ⬇️
Windows 66.40% <76.92%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...s/embeddable/public/lib/panel/embeddable_panel.tsx 63.44% <0.00%> (-0.69%) ⬇️
...ugins/ui_actions/public/actions/action_internal.ts 47.36% <ø> (ø)
src/plugins/ui_actions/public/index.ts 100.00% <ø> (ø)
...lic/context_menu/build_eui_context_menu_panels.tsx 75.60% <90.90%> (+16.35%) ⬆️

... and 42 files with indirect coverage changes

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

@sikhote
Copy link
Contributor Author

sikhote commented Apr 4, 2023

@ashwin-pc I had responded to all comments (some were duplicates). Please let me know if you have any other feedback, thank you.

P.S. I will add in commit sign-offs as a final step.

@joshuarrrr
Copy link
Member

joshuarrrr commented Apr 10, 2023

@sikhote Some of your commits are missing the sign-off, so DCO won't pass. Can you either squash them or rebase with the signatures?

EDIT - saw your note up top. This PR will also need a changelog entry.

@sikhote
Copy link
Contributor Author

sikhote commented Apr 17, 2023

Will do both of those soon 👍

cwperks and others added 15 commits April 23, 2023 21:00
… and show filter (opensearch-project#2656)

* Create filter registry for saved object management to make filters extensible

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* WIP on making fetchCounts generic

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* First step at making scroll_counts generic

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Work on getting other filter counts with same object count endpoint

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Get tenant count options to display

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Extend find to work with namespaces for saved objects

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add missing filterFields

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update jest tests

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update saved_objects_table snapshot

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Append index to id to make unique

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add semi-colon

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Fix saved objects table tests with new id scheme

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only append idx on config type to ensure Advanced Settings have a unique id across tenants

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove itemsClone in favor of showing only Advanced Settings of current tenant

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Revert snapshots in table.test.tsx

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add additional parse_query test

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add comma

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Create namespaceRegistry to decouple security dashboards plugin and osd core

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add ability to register an alias

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update parse query and add to CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove commented out code

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Address code review comments

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Override i18n if alias is regitered

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
…ls to not destroy on unmount

Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
…h-project#2638)

x-pack references in the code are removed as per the given files in opensearch-project#2517

Issue Resolved:
opensearch-project#2517

Signed-off-by: vimal K <vimalinfo10@gmail.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…-project#2681)" (opensearch-project#2694)

This reverts commit 887093d.

Now that downstream plugins and projects are unblocked from builds, we want to make these changes following our standard processes and automated checks

Signed-off-by: Josh Romero <rmerqg@amazon.com>

Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…-project#2538)

* Add MD design documents, including high level design, user stories, client management detailed design

Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* Adds helper functions, @osd/cross-platform, to work around the differences of platforms

Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…PRs (opensearch-project#2726)

Signed-off-by: Miki <amoo_miki@yahoo.com>

Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* Enable visbuilder by default

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* Adds changelog entry

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
kavilla and others added 15 commits April 23, 2023 21:00
Fixes a small typo in TSVB README.md file.

Issue:
n/a

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Ran npx browserslist@latest --update-db to update caniuse package so
the integration tests will pass.

Issue:
n/a

Will be fixed in:
opensearch-project#2329

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…nsearch-project#3533)

Bump vega from 5.22.1 to 5.23.0. This will also bump vega-function
from 5.13.0 to 5.13.1.

Issue Resolved:
opensearch-project#3526
opensearch-project#3525

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Enable the downloading of Darwin for running the command
`yarn opensearch snapshot`.

Darwin is not officially supported but snapshots are being
built here:
https://build.ci.opensearch.org/job/distribution-build-opensearch/

Issue resolved:
opensearch-project#2944

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* Follow-up from opensearch-project#3018

Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* Improved typings

Passing this first param to `CoreSetup` generic would set a proper type to `depsStart` returned from `core.getStartServices()`

Signed-off-by: Aleksandar Djindjic <djindjic@gmail.com>

* Nit - rename example deps prop

Signed-off-by: Josh Romero <rmerqg@amazon.com>

---------

Signed-off-by: Aleksandar Djindjic <djindjic@gmail.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Miki <miki@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…istence (opensearch-project#3495)

* Add metric to metric, bucket to bucket aggregation persistance

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* Add logics to avoid exceeding the max count for each schema field

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* Add changelog entry

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* addressing comments

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* Add unit tests for functions in use_persisted_agg_params

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* rewrite unit tests to test the entire functionality

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

---------

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* docker dev set up folder

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* add curl commands to set up and copy the files

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* changelog changes

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* addressing comments

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* add docker link to the developer guide

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* Add notes on docker desktop

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* publish the docker image to docker hub

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

* add docker exec as another option

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>

---------

Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <julian.labatut@etu.umontpellier.fr>
Co-authored-by: Miki <miki@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <suzhou@amazon.com>

---------

Signed-off-by: suzhou <suzhou@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
* adds ui actions explorer

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* updates UI actions readme

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* updates UI actions readme again

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* cleans up developer examples view

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

* adds changelog

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

---------

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
opensearch-project#3619)

Issue Resolved:
opensearch-project#3618

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
Signed-off-by: David Sinclair <david@sinclair.tech>
@sikhote
Copy link
Contributor Author

sikhote commented Apr 24, 2023

@sikhote sikhote closed this Apr 24, 2023
@kavilla kavilla added v2.9.0 and removed v2.8.0 labels Jun 1, 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.