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.

Sign up for GitHub

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

Jump to bottom

[Menu API] All our drop down menus should use the new menu api #3607 #3620

Merged
merged 11 commits into from Mar 29, 2021
Merged

[Menu API] All our drop down menus should use the new menu api #3607 #3620

merged 11 commits into from Mar 29, 2021

Conversation

nikhilmandlik
Copy link
Contributor

@nikhilmandlik nikhilmandlik commented Dec 23, 2020 •
edited

Resolves:

resolves: #3607

TODOs:

  • fix size of super menu (menu with description) @charlesh88

  • May need to add text-transform: capitalize; on font-style menus @charlesh88
    Screen Shot 2021-01-08 at 3 24 10 PM

  • Create menu

  • Time conductor menus (ConductorMode.vue, ConductorTimeSystem.vue, ConductorHistory.vue)

  • Inspector Font style menus (FontStyleEditor.vue)

Notes:

Identified following menus:

  1. Tree right click context menu (already done)
  2. Notebook Snapshot menu (already done)
  3. View Change menu (already done)
  4. Display layout alphanumeric context click menu (already done)
  5. Display layout Object frame three-dot-menu (already done)
  6. Flex layout Object frame three-dot-menu (already done)
  7. LAD table row (already done)
  8. Telemetry table row (already done)
  9. BrowseBar three-dot-menu (already done)
  10. Preview header three-dot-menu (already done)

Author Checklist:

Check Passed
Changes address original issue? Y
Unit tests included and/or updated with changes? N/A
Command line build passes? Y
Changes have been smoke-tested? Y

@nikhilmandlik nikhilmandlik force-pushed the dropdown-menu branch 2 times, most recently from 67556ea to 49593ca Compare December 24, 2020 23:11
@nikhilmandlik nikhilmandlik force-pushed the dropdown-menu branch 5 times, most recently from 3018575 to d266e99 Compare January 8, 2021 23:20
@nikhilmandlik nikhilmandlik marked this pull request as ready for review January 13, 2021 23:03
deeptailor
deeptailor previously requested changes Jan 22, 2021
View reviewed changes
Copy link
Member

@deeptailor deeptailor left a comment

Choose a reason for hiding this comment

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

This looks great, just one tiny nitpick

src/ui/layout/CreateButton.vue
@@ -114,7 +75,7 @@ export default {
return this.openmct.objects.get(this.openmct.router.path[0].identifier)
.then((currentObject) => {
let legacyContextualParent = this.convertToLegacy(currentObject);
let legacyType = this.openmct.$injector.get('typeService').getType(item.key);
let legacyType = this.openmct.$injector.get('typeService').getType(key);
Copy link
Member

Choose a reason for hiding this comment

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

we can use the new types api (openmct.types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateAction (openmct/platform/commonUI/edit/src/creation/CreateAction.js) requires legacyType, so this might not work unless I am missing something.

@deeptailor
Copy link
Member

@nikhilmandlik - Once you fix conflicts and make the one suggested change, i will merge

charlesh88
charlesh88 requested changes Feb 27, 2021
View reviewed changes
Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

A change is needed to support menu buttons, using CSS to capitalize won't be enough.

Screen Shot 2021-02-26 at 4 31 50 PM

Menu buttons need to display the selected item's label, not its value.

@nikhilmandlik nikhilmandlik force-pushed the dropdown-menu branch 5 times, most recently from af9d201 to 128b8f4 Compare March 26, 2021 01:40
@nikhilmandlik
Copy link
Contributor Author

A change is needed to support menu buttons, using CSS to capitalize won't be enough.

Screen Shot 2021-02-26 at 4 31 50 PM

Menu buttons need to display the selected item's label, not its value.

updated.

@akhenry akhenry changed the title [Menu API] All our drop down menu's should use the new menu api #3607 [Menu API] All our drop down menus should use the new menu api #3607 Mar 29, 2021
@akhenry
Copy link
Contributor

akhenry commented Mar 29, 2021

@charlesh88 @nikhilmandlik This good to go?

@akhenry
Copy link
Contributor

akhenry commented Mar 29, 2021

Reviewer Checklist

  1. Changes appear to address issue? Y
  2. Appropriate unit tests included? Y
  3. Code style and in-line documentation are appropriate? Y
  4. Commit messages meet standards? Y
  5. Has associated issue been labelled unverified? (only applicable if this PR closes the issue) Y

@akhenry akhenry merged commit f9bd31d into master Mar 29, 2021
@akhenry akhenry deleted the dropdown-menu branch March 29, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesh88 charlesh88 charlesh88 approved these changes

@akhenry akhenry Awaiting requested review from akhenry

@davetsay davetsay Awaiting requested review from davetsay

@jvigliotta jvigliotta Awaiting requested review from jvigliotta

@shefalijoshi shefalijoshi Awaiting requested review from shefalijoshi

@deeptailor deeptailor Awaiting requested review from deeptailor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Menu API] All our drop down menu's should use the new menu api
4 participants
@nikhilmandlik @deeptailor @akhenry @charlesh88