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

bug: wrapped menu does not work with content property #16304

Closed
iTEEECH opened this issue Nov 12, 2018 · 20 comments
Closed

bug: wrapped menu does not work with content property #16304

iTEEECH opened this issue Nov 12, 2018 · 20 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@iTEEECH
Copy link

iTEEECH commented Nov 12, 2018

Ionic Info

Ionic:

   ionic (Ionic CLI)             : 4.3.1 (/usr/local/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.0.0-beta.15
   @angular-devkit/build-angular : 0.8.7
   @angular-devkit/schematics    : 0.8.7
   @angular/cli                  : 6.2.7
   @ionic/angular-toolkit        : 1.1.0

System:

   NodeJS : v8.11.3 (/usr/local/bin/node)
   npm    : 6.4.1
   OS     : macOS

Describe the Bug

When putting the menu inside a custom component, (app-menu) to make the app component less busy, I get this result :

https://ibb.co/kUDFOV

But normally, I have to have the initial result :

https://ibb.co/cFcLqA

Steps to Reproduce

1 - Generate a new app with menu
2 - Put the ion-menu in the app-menu
<ion-menu contentId="content1" > <ion-header> <ion-toolbar> <ion-title>Menu</ion-title> </ion-toolbar> </ion-header> <ion-content> <ion-list> <ion-menu-toggle auto-hide="false" *ngFor="let p of appPages"> <ion-item [routerDirection]="'root'" [routerLink]="[p.url]"> <ion-icon slot="start" [name]="p.icon"></ion-icon> <ion-label> {{p.title}} </ion-label> </ion-item> </ion-menu-toggle> </ion-list> </ion-content> </ion-menu>
3 - Put the following in app.component:
<ion-app> <ion-split-pane> <app-layout-menu contentId="content1" [content1]="content"></app-layout-menu> <ion-router-outlet id="content1" main [swipeBackEnabled]="true"></ion-router-outlet> </ion-split-pane> </ion-app>ion

Thank you for your answers !

@ionitron-bot ionitron-bot bot added the triage label Nov 12, 2018
@iTEEECH
Copy link
Author

iTEEECH commented Nov 28, 2018

Hi
Nobody has any solution for this bug ?

thank you so much !

@paulstelzer
Copy link
Contributor

paulstelzer commented Dec 7, 2018

Thanks for your issue! That seems to be like a bug. A workaround would be this:

<ion-app>
  <ion-split-pane when="lg">
    <ion-menu #leftSidebar menuId="left" type="reveal" side="start">
      <app-sidebar-menu></app-sidebar-menu>
    </ion-menu>

    <ion-router-outlet main></ion-router-outlet>

  </ion-split-pane>
</ion-app>

@paulstelzer paulstelzer added type: feature request a new feature, enhancement, or improvement and removed triage labels Dec 7, 2018
@iTEEECH
Copy link
Author

iTEEECH commented Dec 10, 2018

Thank you for your answer ! So it is impossible to apply the principle of angular and to cut the components ?

@paulstelzer
Copy link
Contributor

Okay, sry. I've updated my answer above. It's a bug, you are right :)

@paulstelzer paulstelzer added package: angular @ionic/angular package type: bug a confirmed bug report package: core @ionic/core package and removed type: feature request a new feature, enhancement, or improvement labels Dec 10, 2018
@iTEEECH
Copy link
Author

iTEEECH commented Dec 11, 2018

No problem, I will look in my side If I have the time and wait the next version to test :)

@paulstelzer paulstelzer added type: feature request a new feature, enhancement, or improvement and removed package: angular @ionic/angular package type: bug a confirmed bug report labels Dec 11, 2018
@paulstelzer
Copy link
Contributor

paulstelzer commented Dec 11, 2018

I've spent some time with it today. At the moment it's not possible to put it inside a child component in fact of the following:

https://github.com/ionic-team/ionic/blob/master/core/src/components/split-pane/split-pane.tsx#L135

He is looking if the split pane is the parent element. If you put it inside a child component, this will lead to false inside the ion-menu. It can be fixed by looking through another parentElement like so:

  private isPane(element: HTMLElement): boolean {
    if (!this.visible) {
      return false;
    }

    // If element has no parent element, return false
    const parentElement = element.parentElement;
    if (!parentElement) {
      return false;
    }

    // Parent element not matches the element, but it can has another parent element
    // In case the component is inside another component
    if (parentElement && parentElement !== this.el) {
      return this.isPane(parentElement);
    }
    return parentElement === this.el
      && element.classList.contains(SPLIT_PANE_SIDE);
  }

This would lead that it returns true on the ion-menu. But we need to stop this after some circles otherwise he would walk through all elements (not good). But now we have another issue:

https://github.com/ionic-team/ionic/blob/master/core/src/components/split-pane/split-pane.scss#L36

As you can see ion-menu has to be a child of split-pane

Now you could say: And if we remove this child selector? Problem would be, if you have more than one split-pane, then another menu would become visible

So split-pane has to be refactored. So it's a feature request, not a bug :)

@WhatsThatItsPat
Copy link

I often have the use case where I have a main root side menu on the left, which is ever-present in the app. Then specific page(s) will have a right "menu."

I hesitate to call the right ones menus; they are sub-functionality for a specific pages (and should be easily accessible from the page's scope/code). It's something that a popover could probably do, but a menu on the right feels better and swiping from the right edge is nice. Maybe there should be a distinction between menus and drawers, with the latter being sub-components of pages.

@iTEEECH
Copy link
Author

iTEEECH commented Dec 20, 2018

yes why not ... personally I find the menu component poorly designed.

why not allow to put it in a proper component to him directly ? especially to simplify unit tests.

I take the example of my project where I have several roles and I initialize the menu according to the roles.

I also encounter the problem that I can not activate the menu once authenticated with a * ngIf because the expression false true is already verified because the menu is in the app.component

@Eraldo
Copy link

Eraldo commented Jan 18, 2019

I too am trying to get my "drawers" (side-menus within pages) to work.
I was not able to get it to work with version 4.0.0-rc.2:

Ionic:

   ionic (Ionic CLI)             : 4.8.0 (/usr/local/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.0.0-rc.2
   @angular-devkit/build-angular : 0.12.2
   @angular-devkit/schematics    : 7.2.2
   @angular/cli                  : 7.2.2
   @ionic/angular-toolkit        : 1.2.2

System:

   NodeJS : v11.0.0 (/usr/local/Cellar/node/11.0.0/bin/node)
   npm    : 6.5.0
   OS     : macOS Mojave

I keep getting this error message:

Menu: must have a "content" element to listen for drag events on.

@Yogeshjuya1993
Copy link

Yogeshjuya1993 commented Feb 1, 2019

Please try....
app.component.html
<ion-app> <ion-menu contentId="content" side="start"> <ion-header> <ion-toolbar> <ion-title>Menu</ion-title> </ion-toolbar> </ion-header> <ion-content> menu stuff </ion-content> </ion-menu> <ion-router-outlet id="content" main></ion-router-outlet> </ion-app>

Home.html

<ion-header> <ion-toolbar color="primary"> <ion-menu-button autoHide="false"></ion-menu-button> </ion-toolbar> </ion-header>

https://stackoverflow.com/questions/53657275/ionic-4-adding-side-menu

@PurpleEdge2214
Copy link

PurpleEdge2214 commented Feb 23, 2019

I've been trying to put the side menu into a separate component for a couple of days, and found the following seems to work?

In app.component.html ...
<ion-app> <app-sidemenu></app-sidemenu> </ion-app>

In sidemenu.component.html ...
`<ion-split-pane>
<ion-menu contentId="leftMenu">
\
<ion-toolbar>
<ion-title>Menu
</ion-toolbar>
</ion-header>

<ion-content>

  <ion-list>
    <ion-menu-toggle auto-hide="false" *ngFor="let p of appPages">
      <ion-item [routerDirection]="'root'" [routerLink]="[p.url]">
        <ion-icon slot="start" [name]="p.icon"></ion-icon>
        <ion-label>
          {{p.title}}
        </ion-label>
      </ion-item>
    </ion-menu-toggle>
  </ion-list>

</ion-content>
`

i.e. move the entire contents of the split pane to the menu component, including ion-router-outlet.

There is a working example here...

https://github.com/PurpleEdge2214/Ionic4-Side-Menu-as-a-Component---Example

@Eraldo
Copy link

Eraldo commented May 8, 2019

@Yogeshjuya1993, @PurpleEdge2214

I don't understand how this solves the issue of wanting to have "drawers"?
Meaning: To have different side-menus in different lazy loaded pages.
Does it?

(In any way I thank you for sharing your insights.)

@liquidcms
Copy link

liquidcms commented Jun 12, 2019

Is the final answer here that you can't put a side menu is a custom component?

Going from @Yogeshjuya1993's comment i have this in app.component.html

<ion-app>
  <ion-menu contentId="content" side="start">
    <ion-header>
      <ion-toolbar>
        <ion-title>Menu</ion-title>
      </ion-toolbar>
    </ion-header>
    <ion-content> menu stuff </ion-content>
  </ion-menu>
  <ion-router-outlet id="content" main></ion-router-outlet>
</ion-app>

and in my custom header component.html i have this:

<ion-header class="ion-text-center ion-padding">
    <ion-buttons slot="start">
      <ion-menu-button autoHide="false"></ion-menu-button>
    </ion-buttons>
  <img src="../assets/img/header.png">
</ion-header>

then, on every page i want my header (with side menu button), i add this:
<app-header></app-header>

it almost works. On the start page when app runs, it works (except menu button is hidden when menu opens); but on every subsequent page, the button shows but doesn't do anything (and no errors); and returning to start page, it also no longer works.

@liquidcms
Copy link

Hmm.. i had MenuController imported on one of my pages; this may have been breaking it.

@speence
Copy link

speence commented Jul 15, 2019

It's too bad that the functionality has changed since v3. With a v3 project it was straight forward to have ion-menu components on multiple pages/components of the application. This provided more flexibility with the contents of the menu, as well as functionality. It seems that the new implementation of this component intends for ion-menu to only be used for routing.

I agree with @iTEEECH, I find the way that this component has been designed to be poor. The implementation for v3 provided far more flexibility

@ishan123456789
Copy link

ishan123456789 commented Jun 6, 2021

Any update on this? I'm on v5 and I get this error in production but not in development mode. On debugging found that it happens with SSR. So might be related to this issue ionic-team/stencil#2119
And as mentioned there having same content-id removes the error but my menu behaves strangely with no menu visible at all

@KANekT
Copy link

KANekT commented Apr 8, 2022

bug is actual with #24907

@liamdebeasi liamdebeasi changed the title bug(v4-menu): Menu: must have a "content" element to listen for drag events on. bug: wrapped menu does not work with content property Jul 26, 2022
@liamdebeasi liamdebeasi removed type: feature request a new feature, enhancement, or improvement package: core @ionic/core package labels Jul 26, 2022
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Jul 26, 2022
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jan 10, 2024

Hi everyone,

Here is a dev build if anyone is interested in testing a proposed fix: 7.6.2-dev.11704922151.1fd3369f

Install example:

npm install @ionic/[email protected] @ionic/[email protected]

Please note that this is an Ionic 8 build and is subject to the Ionic 8 breaking changes.

liamdebeasi added a commit that referenced this issue Jan 15, 2024
Issue number: resolves #16304, resolves #20681

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

Split Pane assumes that Menu is a child of the Split Pane. This means
that CSS selectors and JS queries only check for children instead of
descendants. When a Menu is encapsulated in a component that component
can itself show up as an element in the DOM depending on the
environment. For example, both Angular and Web components show up as
elements in the DOM. This causes the Menu to not work because it is no
longer a child of the Split Pane.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Menu can now be used as a descendant of Split Pane. The following
changes make this possible:
1. When the menu is loaded, it queries the ancestor Split Pane. If one
is found, it sets `isPaneVisible` depending on if the split pane is
visible or not.
2. Any changes to the split pane visibility state after the menu is
loaded will be handled through the `ionSplitPaneVisible` event.
3. A menu is now considered to be a pane if it is inside of a split pane
4. Related CSS no longer uses `::slotted` which only targets children.
The CSS was moved into the menu stylesheets. I consider the coupling
here to be valuable as menu and split pane are often used together. We
also already have style coupling between the two components, so this
isn't anything new.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

There are no known changes to the public API or public behavior.
However, there are two risks here:

1. There is an unknown risk into how this change impacts nested split
panes. This isn't an explicitly supported feature, but it technically
works in Ionic now. We don't know if anyone is actually implementing
this pattern. We'd like to evaluate use cases for nested split panes
submitted by the community before we try to account for this
functionality in menu and split pane.
5. There may be some specificity changes due to the new CSS. CSS was
moved from split pane to menu so it could work with encapsulated
components:

`:host(.split-pane-visible) ::slotted(.split-pane-side)` has a
specificity of 0-1-1

`:host(.menu-page-visible.menu-pane-side)` has a specificity of 0-1-0

There shouldn't be any changes needed to developer code since the
selectors are in the Shadow DOM and developer styles are in the Light
DOM. However, we aren't able to anticipate every edge case so we'd like
to place this in a major release just to be safe.

## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
Dev build: `7.6.2-dev.11704922151.1fd3369f`
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #28801, and a fix will be available in an upcoming release of Ionic Framework. We are shipping this in a major release of Ionic to de-risk some of the internal infrastructure changes we made to resolve this bug. Feel free to continue testing the dev build, and let me know if you run into any issues.

Copy link

ionitron-bot bot commented Feb 14, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests