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

TL Components mega-menu has clientHeight = 0 for sections. #358

Open
kaladay opened this issue Sep 23, 2021 · 0 comments
Open

TL Components mega-menu has clientHeight = 0 for sections. #358

kaladay opened this issue Sep 23, 2021 · 0 comments
Labels
bug Something isn't working patch Development requiring a patch version change

Comments

@kaladay
Copy link
Contributor

kaladay commented Sep 23, 2021

The calculation for getting the client height (element height) results in a multiplication against 0, setting a mobile menu height of 0px.

tamu-library-components/projects/tl-elements/src/lib/tl-mega-menu/tl-mega-menu.component.ts

Line 72 in 6cef470

this.mobileDisplayMaxHeight = `${this.sections.length * this.sectionTitleHeight}px`;

This problem is not obvious because all of the sections are looped over and there just happens to be a few that end up provided a non-zero client height.

tamu-library-components/projects/tl-elements/src/lib/tl-mega-menu/tl-mega-menu.component.ts

Line 63 in 6cef470

addSection(section: TlMegaMenuSectionComponent): void {

The CSS styles wvr-nav-list-component.mobile-layout { is explicitly setting max-height: 0px:

tamu-library-components/projects/tl-elements/src/lib/tl-mega-menu/tl-mega-menu-section/tl-mega-menu-section.component.scss

Line 36 in 6cef470

max-height: 0px;

This results in the observed problem.
When the max-height is removed, the client height is properly calculated.
No problems were observed when removing this for the development of tl-dropdown.
This has not been tested on TL Components.

Should be worth further investigating this to confirm this is not a symptom of some other problem rather than being the problem itself.

Should be worth investigating why this max-height is being set.
@kaladay kaladay added the bug Something isn't working label Sep 23, 2021
@ghost ghost added the patch Development requiring a patch version change label Sep 23, 2021
kaladay added a commit that referenced this issue Sep 24, 2021
@kaladay
Issue 336: Tamu Components should support the TL Dropdown within the …
3c762ff 
…TL Header Bottom Nav, with mobile support.

This implements a TL Dropdown Menu rather than extending the TL Dropdown.
The TL Mega Menu is used as the basis such that TL Dropdown Menu is a heavily simplified version of it.

The differences between TL Mega Menu and TL Dropdown are sufficient enough that makes me believe that implementing this inside of TL Dropdown would overcomplicate things and make code harder to maintain, debug, and use.
There are structural differences between TL Dropdown and TL Dropdown Menu that are necessary at this time and these differences follow the same methodology as the TL Mega Menu.
This implementation is otherwise feature for feature and bug for bug to TL Mega Menu.

This updates the TL Header in regards to areas where the TL Mega Menu is also used.

There are styling concerns with the highlighting of the TL Dropdown Menu items when in mobile view.
The approach taken is to match the top-level link structure of TL Mega Menu (which does not highlight on hover over nor does it use bold text).

I wanted to avoid having the explicit "section" (aka TlDropDownMenuSectionComponent).
However, after some simple experimenting, I was unable to find an easy solution using `children`.

This implementation is mostly true to the design of TL Mega Menu, except in a few key places.
1) There is no recursive menu structure.
2) There are no significant limitations on what can be put inside each row, except for the requirement of `<tl-dropdown-menu-section>` and `<tl-nav-li>` elements.
3) There is no "link" (href) support at this time on the menu itself (the menu items do support links and are expected to have links).
4) The `max-height: 0px;` is not copied from the TL Mega Menu (see issue #358).
5) The mobile menu items are styled against the `<tl-nav-li>` rather than the `<a>` elements (this allows for more flexibility in content within the items).
6) This attempts to process elements only once (storing them as a `const`) in a few places to avoid potential problems with elements changing during use (this is done in very few places thus far).

This fixes some obvious typos in TL Mega Menu (`TLMegamMenuComponent` should be `TlMegaMenuComponent`).
This fixes what is either a typo or stale code in the index for TL Dropdown (There is no `tl-drop-down`, instead use `tl-dropdown`).

The following may be dropped inside the appropriate TL Header section (inside the Bottom Navigation) to test the behavior.
```
      <wvre-nav-li>
        <tl-dropdown-menu menu-title="Dropdown Menu">
          <tl-dropdown-menu-section>
            <tl-nav-li>
              <a href="#1">
                <wvre-text value="Example 1"></wvre-text>
              </a>
            </tl-nav-li>
          </tl-dropdown-menu-section>
          <tl-dropdown-menu-section>
            <tl-nav-li>
              <a href="#2">
                Example 2
              </a>
            </tl-nav-li>
          </tl-dropdown-menu-section>
          <tl-dropdown-menu-section>
            <tl-nav-li>
              <wvre-text value="Example 3"></wvre-text>
            </tl-nav-li>
          </tl-dropdown-menu-section>
          <tl-dropdown-menu-section>
            <tl-nav-li>
              Example 4
            </tl-nav-li>
          </tl-dropdown-menu-section>
        </tl-dropdown-menu>
      </wvre-nav-li>
```
@kaladay kaladay mentioned this issue Sep 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working patch Development requiring a patch version change
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kaladay