Enterprise: Soho Tabs (moduleTabs) regression issues

[whernebrink]

Describe the bug
Support for having appMenuTriggerTextAudible: true was added a month ago, which has the following issues in our app (which uses enterprise-ng).

1. Icons (remove - X) are missing from tabs that are dismissible
2. You get this error in the log (although the application menu trigger still works and it looks ok): ERROR TypeError: Cannot read property 'modifyTriggers' of undefined

To Reproduce
Steps to reproduce the behavior:

1. Clone and run the following enterprise-ng-quickstart where I have added app menu and module tabs with dismissible tabs, as well as set the following tabsOptions to appMenuTrigger:false, appMenuTriggerTextAudible: false as default.
   https://github.com/whernebrink/module-tabs-issue
2. Click the button for adding a tab
3. A dismissible tab is added - and you can see the X icon to dismiss the tab
4. Go to the code and set appMenuTrigger:true, appMenuTriggerTextAudible: true
5. In browser console you will see: ERROR TypeError: Cannot read property 'modifyTriggers' of undefined at Tabs.renderHelperMarkup (sohoxi.js:78188)
6. Click the button for adding a tab
7. You will now see a tab, but the icon to dismiss the tab is now missing.

Expected behavior
No error in log, dismiss icon still available.

Version

• ids-enterprise-ng: v9.1.0 (uses ids-enterprise: v4.36.0)

Screenshots

https://user-images.githubusercontent.com/73601152/104440370-8e8b1d80-5592-11eb-9392-683b90751c9a.mp4

Platform

• Infor M3
• Mac OS Catalina
• Browser Name: Chrome
• Browser Version: Version 87.0.4280.88 (Official Build) (x86_64)

[tmcconechy]

@EdwardCoyle can you take a look at this when you have a moment and let us know what you think. I forget why we changed this type back and forth

https://github.com/infor-design/enterprise/blame/d751fdeee6a7bf2e6099be12d6fcf1e56eb04e7c/src/components/tabs/tabs.js#L37

[whernebrink]

@tmcconechy the appMenuTriggerTextAudible was actually something I put in a ticket to have (i.e. the ability to have a menu trigger without a text value. Currently we set appMenuTriggerText to ' ').

It is on line 576 it crashes now where $('#application-menu').data('applicationmenu')is undefined.
$('#application-menu').data('applicationmenu').modifyTriggers([appMenuTrigger.children('a')]);

https://github.com/infor-design/enterprise/blame/d751fdeee6a7bf2e6099be12d6fcf1e56eb04e7c/src/components/tabs/tabs.js#L576

Also, team label for this issue should be M3 something, not stratus if I'm not missing something. :)

[EdwardCoyle]

@whernebrink had a chance to look at this example... In order to properly link the Application Menu to the Tabs' trigger button, this line is necessary:

$('#application-menu').data('applicationmenu').modifyTriggers([appMenuTrigger.children('a')]);

After debugging I can see in the sample that the Tabs component is invoking and setting up ahead of the Application Menu. I'm not sure why that would be the case in this sample (seems like app menu is defined in the template ahead of the Tabs... should be setup ahead of Tabs?). In standard IDS components (outside of angular) we control the initialization process and setup the Application Menu ahead of most other components.

I'm hesitant to remove that line that connects the components, since that would remove any automatic linking. Is it possible to change the order of initialization in your application/setup so the App Menu initializes first?

[whernebrink]

Hi @EdwardCoyle, thanks for looking into it!

After debugging I can see in the sample that the Tabs component is invoking and setting up ahead of the Application Menu. I'm not sure why that would be the case in this sample (seems like app menu is defined in the template ahead of the Tabs... should be setup ahead of Tabs?).

I think it's a good thing this issue was caught and brought up! In Angular, the children (e.g. soho-tabs inside my app-tabs component) are initialized first. It first need to initialize all child components/views before the root one. Kinda counterintuitive, one may think (although, makes sense when you think about it some more). But this is important to know, so that we do not make assumptions about what _will_ or _should_ be initialized first in ids-enterprise.
I have updated the repository I shared in this ticket and added a appSpy directive to the components which logs out the component, when onInit/afterViewInit etc are called, and as you can see, the soho-tabs's afterViewInit is called before soho-application-menu's afterVIewInit (which is the second from the bottom), which should explain why this issue of initialization order occurs?

We have, as many others, a big codebase. We need to have a good structure and to separated our code into smaller parts in order to be able to maintain it. We’re using angular libraries for feature-modules, shared ui components etc. which facilitate us breaking up the application in these smaller parts, which also has the benefit for us to be able to reuse functionality and/or components in other applications, e.g. our m3-tabs component, which wraps the soho-tabs[moduleTabs=true].

With that background to the architecture being said, I have set the following application architecture for us (M3):

• app.component.html
  
  
  
  • nav[soho-application-menu]
  
  
  • div.page-container
    
    
    
    
    
    • main#maincontent
      
      
      
      
      
      
      
      • (domain specific feature module “libs/m3-client/feature-main-controller” library)
      
      
      
      
    
    
    
  
  
  • div[soho-personalize]:
  
  

++++++++++++++++++++++++++++++++

// resides inside feature-main-controller library

• m3-main.controller.component.html
  
  
  
  
  

++++++++++++++++++++++++++++++++

// resides inside shared/ui/m3-components library

• m3-tabs.component.html
  
  
  
  • div[soho-tabs]
    
    
    
    
    
    • div[soho-tab-list-container]
      
      
      
      
      
      
      
      • div[soho-tab-panel] *ngFor=“let tab of tabItems”
        
        
        
        
        
        
        
        
        
        • 
          
          
          
          
      
      
      
      
    
    
    
    • …etc
    
    
    
  
  

++++++++++++++++++++++++++++++++

This architecture of ours was working prior to (i.e. not getting any error logs in console):
https://github.com/infor-design/enterprise/issues/4590

In 4590, I just wanted to be able to not have a text value for for the menu trigger button, without impacting the actual trigger button width (which is the case when we provide appMenuTriggerText = ‘ ‘;).

I’d much rather go back to where we were, and be able to keep our application architecture as is, than having to work around this issue with soho components initialization order.

If we, the consumers of ids-enterprise-ng, have to worry about soho-components not being initialized in the right order due to where we decide to use the soho-components (e.g. wrapped in a application specific component or shared-ui component), it will not be the best of developer experiences, I'd say. However, If that is the case, then we need a clear documentation/guideline about which components are dependent on others, and which ones needs to be initialized in what order...

I hope we will just revert 4590 if that ticket is not possible to solve without having to re-architect our application (which, as I mentioned above) worked just fine prior to me logging that issue… :)

[tmcconechy]

@EdwardCoyle might make sense to revert 4590 if that in fact caused this and not something else? EIther that or maybe we can find a simpler Css based way to solve it. Maybe with :empty css selector or min width or something?

Not sure of all the details because im not 100% up on this issue

[EdwardCoyle]

We shouldn't have to revert #4590. This is something we can solve with a small check in Tabs I think. The difficulty is that the NG environment is setup to be more "manually" scaffolded (hence @whernebrink's issue with the ordering) , but anyone consuming the ES6-only components depends on the automatic initialization, so we need to ensure both work.

[janahintal]

QA Passed. Working as expected. Thank you.