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 Anywhere] add category option for context menus #4146

Conversation

sikhote
Copy link
Contributor

@sikhote sikhote commented May 26, 2023

This includes these changes #4144 and the changes already merged here #3924

Description

This allows a group within a context menu to be categorized and presented adjacent to other groups. Typically, a group is divided by a separator from other groups, but sometimes we want to present groups related to the same feature next to each other. By adding a category option to a group, we can accomplish this.

Issues Resolved

#4073

Screenshot

Screenshot 2023-05-25 at 10 24 26 PM

Testing the changes

These changes can be tested using the regular testing suite, or by visiting the example page locally at http://localhost:5601/app/uiActionsExplorer

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
Copy link
Contributor Author

sikhote commented May 26, 2023

FYI @lezzago

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #4146 (51722f6) into feature/feature-anywhere (ec4c1df) will increase coverage by 0.08%.
The diff coverage is 78.78%.

❗ Current head 51722f6 differs from pull request most recent head bd0c27d. Consider uploading reports for the commit bd0c27d to get more accurate results

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #4146      +/-   ##
============================================================
+ Coverage                     66.44%   66.53%   +0.08%     
============================================================
  Files                          3247     3247              
  Lines                         62553    62571      +18     
  Branches                       9659     9667       +8     
============================================================
+ Hits                          41563    41630      +67     
+ Misses                        18676    18625      -51     
- Partials                       2314     2316       +2     
Flag Coverage Δ
Linux 66.47% <78.78%> (+0.03%) ⬆️
Windows 66.47% <78.78%> (?)

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

Impacted Files Coverage Δ
...ugins/ui_actions/public/actions/action_internal.ts 47.36% <ø> (ø)
...lic/context_menu/build_eui_context_menu_panels.tsx 75.75% <78.78%> (+16.49%) ⬆️

... and 9 files with indirect coverage changes

@lezzago lezzago self-requested a review May 26, 2023 22:03
@ohltyler
Copy link
Member

@sikhote could we prioritize merging #4144 first? Then we can simply pull the changes into the feature branch without having multiple PRs.

Comment on lines 1 to 29
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Any modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/

/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
Copy link
Member

Choose a reason for hiding this comment

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

For new files we can use the shortened header:

/*
 * Copyright OpenSearch Contributors
 * SPDX-License-Identifier: Apache-2.0
 */

Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

Just one minor comment. Left the same on the original PR.

Signed-off-by: David Sinclair <[email protected]>
@sikhote
Copy link
Contributor Author

sikhote commented Jun 1, 2023

This branch is now updated with changes from #4144, mostly incase we are not ready to pull main branch into feature/feature-anywhere

@joshuarrrr
Copy link
Member

@sikhote This PR still has conflicts to resolve. The unit tests are also failing: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/5141644713/jobs/9254367815?pr=4146

@ohltyler
Copy link
Member

ohltyler commented Jun 8, 2023

@sikhote This PR still has conflicts to resolve. The unit tests are also failing: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/5141644713/jobs/9254367815?pr=4146

Now that #4144 has merged to main, I think a merge of main -> feature branch should be sufficient.

@sikhote
Copy link
Contributor Author

sikhote commented Jun 8, 2023

Just a heads up, this was always more of a convenience branch until feature/feature-anywhere was able to pull in main—if still needed, you might ask @lezzago or another to take over this PR—I'm off the project for now 😬

@joshuarrrr
Copy link
Member

Closing as no longer need (no diff with feature branch)

@joshuarrrr joshuarrrr closed this Jun 23, 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.

4 participants