headlamp icon indicating copy to clipboard operation
headlamp copied to clipboard

Improve app menus

Open joaquimrocha opened this issue 2 years ago • 5 comments

This PR does:

  • Simplify how menus are built in app's main.ts
  • Allow to keep menus translated even after they've been altered by a plugin (default menus that is)
  • Remove the "Headlamp" word from one of the menus
  • Translate Help menu
  • Load certain menus only after plugins are loaded

Testing:

  • [ ] Pre-testing: Make sure you have the app-menus plugin in place
  • [ ] Launch app and watch the menus: verify that the "Help" menu is not initially loaded until a few fractions of a second (till plugins are loaded)
  • [ ] Change the language of the app: verify that even with the overridden menu, the default menu items' labels have been updated
  • [ ] Verify that the Help menu is now translated as well
  • [ ] On Mac, verify that the "Headlamp" menu now shows "Hide" instead of "Hide Headlamp"

joaquimrocha avatar Aug 08 '22 13:08 joaquimrocha

Load certain menus only after plugins are loaded

Why?

illume avatar Aug 10 '22 06:08 illume

I tried running the app with the app-menu example plugin but it is not loading for me.

This plugin is broken. If you move the logic outside of the class, it'll work.

joaquimrocha avatar Aug 10 '22 08:08 joaquimrocha

Load certain menus only after plugins are loaded

Why?

Because we don't want plugins to e.g. change the website for the help menu only to have the users see the original/default URL for example.

joaquimrocha avatar Aug 10 '22 08:08 joaquimrocha

re: moving the menu stuff into a menu.ts...

Can you please move all the menus related stuff into a menus.ts? There's already too much stuff in main.ts, and that would make this much easier to understand.

Do you want to do that in here, or we can do it later?

Regarding the plugin...

I tried running the app with the app-menu example plugin but it is not loading for me.

I didn't yet look into it further than "npm run dev works, and npm run dev-only-app fails". @ashu8912 Does npm run dev work for you? I can look into this further tomorrow.

illume avatar Aug 10 '22 20:08 illume

re: moving the menu stuff into a menu.ts...

Can you please move all the menus related stuff into a menus.ts? There's already too much stuff in main.ts, and that would make this much easier to understand.

Do you want to do that in here, or we can do it later?

I missed your suggestion, sorry. I want to split the menu definition, but I have an idea for making it even easier to replace (having the labels just be text, not i18n.t and still parse them into the translation catalog). I think regardless of how we split it, it's not worth the risk of doing it at this point. I prefer to merge as is, because it's tested, and then we refactor the code.

Regarding the plugin...

I tried running the app with the app-menu example plugin but it is not loading for me.

I didn't yet look into it further than "npm run dev works, and npm run dev-only-app fails". @ashu8912 Does npm run dev work for you? I can look into this further tomorrow.

npm run dev works with a prebuilt frontend/build. Did you double check whether it works if you make && cd app && npm run dev? It should fail just like the only-app variant.

joaquimrocha avatar Aug 11 '22 10:08 joaquimrocha