Mango Menu Redesign

[dailenisgonzalez]

• Top menu re-design: https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1668

• Add the top-level menu control to Mango: https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1666

• Add the Submenu Control to Mango: https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1667

• Update all modules menu item inject url and all links in all pages: https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1844

• Adjust format of date/time under the logo and add the Time Zone https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1856

• Fixes for the Help Submenu https://radixiot.atlassian.net/jira/software/c/projects/PI/boards/53?modal=detail&selectedIssue=PI-1903

With the new menu updates we need to update the locations of the menu items in this modules: https://github.com/MangoAutomation/dashboards/pull/48 https://github.com/MangoAutomation/ma-core-public/pull/1837 https://github.com/MangoAutomation/ma-modules-private/pull/496 https://github.com/MangoAutomation/ma-modules-proprietary/pull/358 https://github.com/MangoAutomation/ma-modules-public/pull/334 https://github.com/MangoAutomation/project-atc-us-towers/pull/347 https://github.com/MangoAutomation/project-encompass-view/pull/16

https://user-images.githubusercontent.com/87542019/227037178-680f14b8-570d-47a2-93fc-6edf0a543049.mov

[pierpuccini]

@dailenisgonzalez By looking at the video in seconds 0.17-0.20, Are the "folders" going to be black shouldn't they match the other items so we can keep some consistency? Also when you hover over a folder I think that that dark blue is way to much for the light pallet. If the design (colors and that sort of stuff) wasn't planed for this issue then disregard my comment :)

[dailenisgonzalez]

@dailenisgonzalez By looking at the video in seconds 0.17-0.20, Are the "folders" going to be black shouldn't they match the other items so we can keep some consistency? Also when you hover over a folder I think that that dark blue is way to much for the light pallet. If the design (colors and that sort of stuff) wasn't planed for this issue then disregard my comment :)

Hi good feedback. It wasn't planed for this issue but I think that it will be good to implement I tag @greychrist maybe he can create another task for those changes.

[jazdw]

@dailenisgonzalez @greychrist @terrypacker I think we should strongly reconsider changing all the URLs/state names wherever possible. e.g. changing ui.help -> ui.helps and ui.settings to ui.system doesn't make sense. This will break everyone's bookmarks, start pages, links from customer/user dashboards, external links etc.

[jazdw]

Another comment on the screenshots: The left menu with icon looks good, but the submenu needs some work IMO, there's too much whitespace and the purple text doesn't contrast enough with the dark background.

[greychrist]

Another comment on the screenshots: The left menu with icon looks good, but the submenu needs some work IMO, there's too much whitespace and the purple text doesn't contrast enough with the dark background.

@dailenisgonzalez @greychrist @terrypacker I think we should strongly reconsider changing all the URLs/state names wherever possible. e.g. changing ui.help -> ui.helps and ui.settings to ui.system doesn't make sense. This will break everyone's bookmarks, start pages, links from customer/user dashboards, external links etc.

Okay I can buy that - it seems like it would save us work though my only hesitation is that since the menu structure is changing are we not then introducing complexity for future projects? Just something to chew on. My other question would be to Dailenis - is NOT changing those links feasible? Does the code depend on those links following the structure of the menu or the underlying model that contains the menu information?

[greychrist]

Another comment on the screenshots: The left menu with icon looks good, but the submenu needs some work IMO, there's too much whitespace and the purple text doesn't contrast enough with the dark background.

On this one I would say I disagree that the text isn't contrasting enough, but that's just my opinion. @dailenisgonzalez is it possible to just let that text and the icon be white when in dark mode?

[greychrist]

Another comment on the screenshots: The left menu with icon looks good, but the submenu needs some work IMO, there's too much whitespace and the purple text doesn't contrast enough with the dark background.

@dailenisgonzalez @greychrist @terrypacker I think we should strongly reconsider changing all the URLs/state names wherever possible. e.g. changing ui.help -> ui.helps and ui.settings to ui.system doesn't make sense. This will break everyone's bookmarks, start pages, links from customer/user dashboards, external links etc.

Okay I can buy that - it seems like it would save us work though my only hesitation is that since the menu structure is changing are we not then introducing complexity for future projects? Just something to chew on. My other question would be to Dailenis - is NOT changing those links feasible? Does the code depend on those links following the structure of the menu or the underlying model that contains the menu information?

I spoke with Dailenis about this - we have to make this change because the menu editor depends upon the state name property matching the structure of the menu, also, the menu rendering depends on this structure as well, so when customers upgrade they'll need to update their links - can't get around it. I don't think it's too painful it's just going to have to be a step in the process.

[greychrist]

Okay Dailenis will adjust the menu styles slightly to make that main menu more readable and adjust some sizing. As to the purple used on the dark theme - that we can adjust with Dennis on his theme update if needed. We can discuss. I have a couple other tickets coming out of this review so we will address those items in separate tasks. Thanks everyone for your feedback!

[greychrist]

@jazdw @MertCingoz @pierpuccini @jpuccinit Hey all, where are we at with this PR? We have some tickets that are hung up on this one.

[jazdw]

I spoke with Dailenis about this - we have to make this change because the menu editor depends upon the state name property matching the structure of the menu, also, the menu rendering depends on this structure as well

This is true, some states might need to change if the structure changes. But we should only change the states that we actually need to. Unless I am missing something changing ui.help to ui.helps is super unnecessary and also grammatically incorrect.

@dailenisgonzalez were the screenshots updated? I can't really see any difference.

This looks pretty wonky too:

[dailenisgonzalez]

@jazdw I changed to "helps" because one of the request on my task was move "help" inside new category that not only includes "help" also includes "Online help", "docs", "API Docs" "Mango Examples" and "Help Forum". For that the main category I named 'helps' and inside this contain "ui.helps.onlineHelpDocs", "ui.helps.help" , "ui.helps.examples", "ui.helps.docs" and "ui.helps.helpForum":

[greychrist]

I spoke with Dailenis about this - we have to make this change because the menu editor depends upon the state name property matching the structure of the menu, also, the menu rendering depends on this structure as well

This is true, some states might need to change if the structure changes. But we should only change the states that we actually need to. Unless I am missing something changing ui.help to ui.helps is super unnecessary and also grammatically incorrect.

@dailenisgonzalez were the screenshots updated? I can't really see any difference.

This looks pretty wonky too:

@jazdw - as Dailenis said - the reason she moved those items to the ui.helps heading is because the structure was changed and multiple items including the original 'Help' was moved under a 'Help' heading and they've become submenu items. So the structure had to follow. And I hear what you're saying about 'helps' seeming grammatically incorrect but if you think about a 'help' item as a noun it sort of IS correct. Unless we changed it to helpLinks or something but to me that seemed like overkill. Trust me I'm a grammar nazi myself, so it was a little bit of a struggle but I've convinced myself now that a 'help' is a noun and I feel better about it now.

On the other item, about how odd the Help submenu looks - I am right there with you. Dailenis and I had a deeper dive review on the work done so far and a few new tasks came out of that, one of which was to address that submenu and make it more palateable. The concept of a sub-submenu hasn't been seen yet, so we're working through it now, but there is a task to work on it and it WILL be fixed in an upcoming push.

[pierpuccini]

I think that for future PR's or this one we should make them easier to review, For example theres a lot of white space changes and quote changes that previously wouldn't and shouldn't be allowed and if they are the be allowed we should make one mr for this code formate change. We might all have prettier but not have it set up the right way for the mango coding style.

I suggest that we use husky or some other hook that will format our code from now on.

[greychrist]

I think that for future PR's or this one we should make them easier to review, For example theres a lot of white space changes and quote changes that previously wouldn't and shouldn't be allowed and if they are the be allowed we should make one mr for this code formate change. We might all have prettier but not have it set up the right way for the mango coding style.

I suggest that we use husky or some other hook that will format our code from now on.

@pierpuccini - If there are coding style requirements that were missed then let's address that right now. I completely agree that we should stay within the coding style with any commits we make to this code base 100% and that's what these reviews are for. If there's a way to upload/download prettier settings maybe someone can get those over to @dailenisgonzalez and she can update her commit? Otherwise, let's detail them in this review.

@dailenisgonzalez sorry to give you more work, but it's important that the code style is the same across the application and modules. This is a very mature and wide-ranging application so keeping it in line with other code is crucial to maintainability.

[jazdw]

@dailenisgonzalez Thanks for the additional screenshots, that helps visualize what happened: we added another level to the menus. I get it now.

@greychrist "helps" is still rather jarring to me but I'm not going to fight it, its not really visible to the end user anyway.

On the other item, about how odd the Help submenu looks - I am right there with you. Dailenis and I had a deeper dive review on the work done so far and a few new tasks came out of that, one of which was to address that submenu and make it more palateable. The concept of a sub-submenu hasn't been seen yet, so we're working through it now, but there is a task to work on it and it WILL be fixed in an upcoming push.

OK thanks. I do suggest we try and get these things right before merging in the future though. I understand there is pressure to close tickets but if we merge incomplete work there will inevitably be situations where it is not fixed before a release.

[greychrist]

@dailenisgonzalez Thanks for the additional screenshots, that helps visualize what happened: we added another level to the menus. I get it now.

@greychrist "helps" is still rather jarring to me but I'm not going to fight it, its not really visible to the end user anyway.

On the other item, about how odd the Help submenu looks - I am right there with you. Dailenis and I had a deeper dive review on the work done so far and a few new tasks came out of that, one of which was to address that submenu and make it more palateable. The concept of a sub-submenu hasn't been seen yet, so we're working through it now, but there is a task to work on it and it WILL be fixed in an upcoming push.

OK thanks. I do suggest we try and get these things right before merging in the future though. I understand there is pressure to close tickets but if we merge incomplete work there will inevitably be situations where it is not fixed before a release.

Thank you @jazdw it's much appreciated. I do agree with you on fixing things before the PR - that's how I would normally do it too, but I get a lot of pushback on that from the Scrum Master side of things that it's better to just create a new task and do the fix there so the work can be tracked better so long as the current code doesn't 'break' anything - this one is sort of borderline you know? If this were a smaller commit I would say let's go back and fix it in this one but I'm trying to push this elephant over a wall and my arms are getting tired lol.

I AM going to request that we fix the coding standards violations for this PR though - there's no sense putting differently-formatted code into this code base only to change it later (and thus have to review it again).

[greychrist]

Also - I like @pierpuccini 's thought on using Husky to enforce coding standards. I'm not familiar with it but we have some new people working on this code base now and coding standards are really important in my opinion. Not trying to put extra work on anybody but as a measure to save the team more work later on maybe we should consider standardizing on something like that? I'm not as knowledgeable as you guys are so I'll just leave it there to think on.

[terrypacker]

@greychrist if you want to add format checks to the PRs for the UI code, can you comment on this issue that we are using for the new mango repo? @pierpuccini same, we are settings this all up currently so now is a good time to make suggestions.

https://radixiot.atlassian.net/browse/PI-1849

[jazdw]

I'm trying to push this elephant over a wall and my arms are getting tired lol.

@greychrist Haha trust me I know what you are talking about with these large PRs.

re. code style, this is not currently enforced anywhere but we should definitely enforce it in Mango 5. Have a look at the JIRA ticket Terry posted, we are planning on enforcing the Java code style using Spotless, I think it probably supports JavaScript/HTML/Typescript too.

@terrypacker @pierpuccini any idea where the code standards for frontend development actually are? I found the Java code style standards, but can't locate the frontend standards in Confluence. I know we use AirBnb's style guide for Javascript with 4 spaces instead of 2.

[jazdw]

Regardless of code style standards we should try not to reformat files when making changes, I think this is what @pierpuccini is talking about

[pierpuccini]

@terrypacker @pierpuccini any idea where the code standards for frontend development actually are? I found the Java code style standards, but can't locate the frontend standards in Confluence. I know we use AirBnb's style guide for Javascript with 4 spaces instead of 2.

The following are also in this google doc

But basically the full abnb style guide, with the only difference being the spacing like you mentioned above, there's other stuff in there such as not to use Angularjs directives but I thinks those would be guidelines.

in order to use the abnb style for mango root I had to do some crazy stuff so If we can get the spotless tool or husky to do all of this it would be great!

[pierpuccini]

Regardless of code style standards we should try not to reformat files when making changes, I think this is what @pierpuccini is talking about

Yeah another file I remember would be

UI/web-src/ui/app.js

[dailenisgonzalez]

@jazdw @pierpuccini I will work on that code format and let you know when is ready for review it, thanks.

[jazdw]

Thanks @pierpuccini for linking to the frontend development guide. @dailenisgonzalez Thanks, let us know.

[dailenisgonzalez]

@jazdw @greychrist @pierpuccini I fixed the code format, and fix the submenu nested items for example "helps" and also updates the video and the screenshots it is ready to review.

[dailenisgonzalez]

@jazdw I resolved the comments. Related to the squash commits I have to squash the commits for task or feature? Because in this case for example the feature is "Mango Menu Redesign" but this includes like 5 different task.

[jazdw]

Related to the squash commits I have to squash the commits for task or feature? Because in this case for example the feature is "Mango Menu Redesign" but this includes like 5 different task.

Squash them down to whatever makes sense, I just don't want a commit which adds .DS_Store then another which removes it. Ideally I think this could be squashed to a single commit.

[dailenisgonzalez]

@jazdw @greychrist I resolved all comments. This pull request depends on this others pull request. I need help in the review too: https://github.com/MangoAutomation/dashboards/pull/48 https://github.com/MangoAutomation/ma-core-public/pull/1837 https://github.com/MangoAutomation/ma-modules-private/pull/496 https://github.com/MangoAutomation/ma-modules-proprietary/pull/358 https://github.com/MangoAutomation/ma-modules-public/pull/334 https://github.com/MangoAutomation/project-atc-us-towers/pull/347 https://github.com/MangoAutomation/project-encompass-view/pull/16

[greychrist]

@jazdw how are we doing here? Seems like she resolved all the comments.

[greychrist]

@greychris yes looks like my comments were resolved. I saw something else but its not a big deal if that is not changed. I agree with @pierpuccini that there are still a lot of formatting and whitespace changes which aren't necessary and make this PR very hard to review since its so broad. I will not hold it up any longer but we really need to make sure this doesn't happen again.

Thanks @jazdw we really appreciate it. If you'd like we can review the whitespace issues in a quick session to help avoid that in future, and again as Pier pointed out, having a standardized tool for formatting for everyone on the team would probably be a big help.

[dailenisgonzalez]

@greychrist @jazdw @pierpuccini thanks for all the review, the formatting and whitespace changes it will not happen again I will take care of that and make small pull request for the next time. This task was complicated I can not push and incomplete menu.