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

AppMenu: Not able to set module tabs appMenuTriggerText to empty string #4590

Closed
whernebrink opened this issue Nov 9, 2020 · 6 comments · Fixed by #4710
Closed

AppMenu: Not able to set module tabs appMenuTriggerText to empty string #4590

whernebrink opened this issue Nov 9, 2020 · 6 comments · Fixed by #4710
Assignees
Labels
team: m3 Issues for m3 and sub teams type: bug 🐛 [2] Velocity rating (Fibonacci)

Comments

@whernebrink
Copy link

Describe the bug
For the module tabs you can provide options to have and set the application menu trigger text:

const TABS_DEFAULT = {
  ...otherStuff,
  appMenuTrigger: false,
  appMenuTriggerText: undefined,
  ...moreStuff,
};

If appMenuTrigger is true but appMenuTriggerText is not set, it will default to Locale.translate('AppMenuTriggerText'). We would like to be able to not have a title text i.e. "" (blank). Currently, if we provide empty string it defaults to 'Menu' due to the following on line 543 in tabs.js:

<span>${this.settings.appMenuTriggerText || Locale.translate('AppMenuTriggerText')}</span>

So we currently specify " " a space which won't evaluate false, but has the side effect of the icon becomes misplaced.

To Reproduce
Steps to reproduce the behavior:

  1. Set appMenuTrigger to true and appMenuTriggerText to "" (empty)
  2. You will have 'Menu' as the trigger text

Expected behavior
When provide empty string i.e. "" there shouldn't be a default.

Version

  • ids-enterprise: 4.35.0-dev

Screenshots
The menu icon becomes misplaced by adding the space:
Screenshot 2020-11-09 at 20 15 14

Platform

  • All

Additional context
Using the nullish coalescing operator (??) (CanIuse) instead of the logical or (||) would let empty string through, but at the same time set the default text appMenuTriggerText
is null or undefined.

<span>${this.settings.appMenuTriggerText ?? Locale.translate('AppMenuTriggerText')}</span>
@tmcconechy tmcconechy added [2] Velocity rating (Fibonacci) type: bug 🐛 labels Nov 9, 2020
@tmcconechy tmcconechy added this to To do in Enterprise 4.36.x (Dec 2020) Sprint via automation Nov 9, 2020
@tmcconechy tmcconechy added the team: m3 Issues for m3 and sub teams label Nov 9, 2020
@tmcconechy tmcconechy changed the title Not able to set module tabs appMenuTriggerText to empty string AppMenu: Not able to set module tabs appMenuTriggerText to empty string Nov 9, 2020
@tmcconechy
Copy link
Member

As an alternate workaround does setting it to &nbsp; work?

@whernebrink
Copy link
Author

whernebrink commented Nov 10, 2020

@tmcconechy Forgot to mention that I've already tested that too, with the same outcome as " ". And I've come to the conclusion that it is probably due to the following style selector (see print screen for applied styles): .tab-container.module-tabs .tab a > span:not(.icon)
I noticed when removing the margin when there isn't a text value, it would give me the desired outcome (in spite of " " or "&nbsp").

So in order for the menu icon to look centered on hover (before one clicks it) when there isn't a trigger text in the span:not(.icon), this margin cannot be applied.

Screenshot 2020-11-10 at 08 24 29

How it looks (on inspec/hover) when the margin is applied:
Screenshot 2020-11-10 at 08 43 02

@whernebrink
Copy link
Author

Also, I forgot to mention: in our code we have to unset the min-width for the application-menu-trigger in order to get the trigger not look huge:

This is how it looks by default if one set the trigger text to " ":
Screenshot 2020-11-10 at 22 34 01

We have to add the following style:

:host::ng-deep {
    [soho-tab-list] {
      .tab.application-menu-trigger {
        min-width: unset;
      }
    }
  }

Ouput:
Screenshot 2020-11-10 at 22 31 29

When also removing the previously mentioned margin, we get the desired output (which I do think should be default if you were to add set " " as value for the appMenuTriggerText:

With removal of margins:

:host::ng-deep {
    [soho-tab-list] {
      .tab.application-menu-trigger {
        min-width: unset;

        span:not(.icon) {
          margin: 0;
        }
      }
    }
  }

Screenshot 2020-11-10 at 22 37 21

@tmcconechy
Copy link
Member

Is this using this example https://master-enterprise.demo.design.infor.com/components/tabs-module/example-app-menu-button.html but just without the Menu text? Or maybe you werent aware of this configuration?

The more i look at this it looks like that example

@whernebrink
Copy link
Author

Correct, I'm using that appMenuTrigger with appMenuTriggerText set to " ". Yep, without the menu text is what we want and also, when the button only consist of the bars, that the width of that trigger button is no bigger than the actual icon.

@EdwardCoyle EdwardCoyle moved this from To do to In progress in Enterprise 4.36.x (Dec 2020) Sprint Dec 22, 2020
EdwardCoyle added a commit that referenced this issue Dec 22, 2020
@EdwardCoyle EdwardCoyle moved this from In progress to Pending Review in Enterprise 4.36.x (Dec 2020) Sprint Dec 22, 2020
@tmcconechy tmcconechy moved this from Pending Review to Ready for QA (beta) in Enterprise 4.36.x (Dec 2020) Sprint Dec 23, 2020
@janahintal
Copy link
Contributor

QA Passed.
Verified in https://master-enterprise.demo.design.infor.com/components/tabs-module/example-app-menu-button-audible.html?layout=nofrills. Used Chrome Reader Extension for verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team: m3 Issues for m3 and sub teams type: bug 🐛 [2] Velocity rating (Fibonacci)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants