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

My impressions on alpha.3 #35

Closed
julianobrasil opened this issue Oct 15, 2017 · 15 comments
Closed

My impressions on alpha.3 #35

julianobrasil opened this issue Oct 15, 2017 · 15 comments

Comments

@julianobrasil
Copy link
Contributor

julianobrasil commented Oct 15, 2017

Look at what I got:

image

As you can notice from the gif above,:

  1. The flickering issue was solved by using the @Input() disableBackdrop
  2. The content of the popover is different for each option in select

The second observation has an hidden implication: as I need one anchor to each sat-popover, I was forced to encapsulate every option content in a component - see the code bellow. The mat-option content is the anchor for the sat-popover - ideally, the anchor would be the mat-option itself not its content - but I'll talk about this later in this issue.

Sorry about the awkward example (bellow) to show my point, but I tried to reproduce approximately what I have in above gif [edited => the code bellow is in stackblitz.com].

<div class="full-width" [satPopoverAnchorFor]="popover" satDisableClick 
    (mouseenter)="openpopover()" (mouseleave)="closepopover()">
  {{message}}
</div>
<sat-popover #popover xPosition="after" [overlapAnchor]="false" [disableBackdrop]="true">
  <div class="popover-class">
      <ng-container *ngFor="let t of myArray; let i=index">
        <br *ngIf="i > 0">
        <strong>
          {{t}}
        </strong>
      </ng-container>
  </div>
</sat-popover>

and in the class:

  /** 
   * I could use the SatPopover here, as it now has the open() and close() properties
   *   but it doesn't solve the problem I'll describe bellow   
   */
  @ViewChild(SatPopoverAnchor) anchor: SatPopoverAnchor;

  @Input() message: string;
  @Input() myArray: any[];

  openpopover() {
    this.anchor.openPopover();
  }

  closepopover() {
    this.anchor.closePopover();
  }

Then I can use this component like this (mat-form-field's width is important to show my point):

<mat-form-field style="width: 400px">
    <mat-select placeholder="Select an option">
        <mat-option *ngFor="let oneArray of arrayOfArrays; let i = index" [value]="oneArray">
            <app-popover [message]="i" [myArray]="oneArray"></app-popover>
       </mat-option>
    </mat-select>
</mat-form-field>
arrayOfArrays = [
  [a,b,c],
  [d,f],
  [g,h,i]
]

As I need an unique anchor for each popover, I made this custom component to encapsulate each of the popover and it's anchor.

As I said earlier, ideally, the anchor should be the mat-option element, but I couldn't figure out how to put it in the custom component. I tried ngProjectAs="mat-option", but apparently mat-option is not a ng-content element:

<mat-select placeholder="Turma">
    <app-popover *ngFor="let oneArray of arrayOfArrays; let i = index" [message]="i" 
         [myArray]="oneArray" ngProjectAs="mat-option"></app-popover>
</mat-select>

As it didn't work, what I managed to do is encapsulating mat-option content along with sat-popover instead of mat-option itself. So I'm using a div inside the custom component to hold the option content and be the anchor for the popover.

This have a side effect: I had to override the mat-option-text class so it takes 100% of the available mat-option width, otherwise the popover would apear in the middle of the mat-option instead of by its side. Even overriding the mat-option-text class, if you look carefully, you can see in the gif that the popover overlapped the mat-select pannel a little bit.

The overlapping is acceptably small, but if I had to make it goes completely outside the panel, I don't kwow what I could do without hacking the component.

The summary of the problem: When I have an undetermined number of dynamically fed sat-popover in a component, I'm not able (at least I couldn't figure out how) to put an anchor in a component directly, without encapsulating the popover, what may have side effects in some specific situations like what is discribed in this issue. This would happen also if I was using another component with a separate trigger element, like mat-menu. The problem is to put the anchor in the correct element, once it is buried inside another custom component.

  1. Is it possible to use another aproach to do what I'm trying to?
  2. Is this a limitation of sat-popover? Or a limitation of mat-option?
  3. Is there any possible workaround?

@willshowell

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Oct 15, 2017

What if we think in a way to send a component as a parameter dynammically, using portals, like mat-snackbar does (#23)? Or maybe a more self-contained approach, like matTooltip (that doesn't need an anchor)? I mean, the way it is built now, working like mat-menu, IMO it's not ready to work with mat-select (or mat-select isn't ready to work with sat-popover, mat-menu or anything like them, that has the possibility to react to hover events).

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Oct 15, 2017

I've built an working example on stackblitz.com: https://stackblitz.com/edit/popover-example

The red color under each option helps to show the problem by marking the sat-popover anchor. You'll have to hover the mouse exactly over the red region in each option to see the popover.

Then, if you uncomment the last lines in styles.scss, you can make it a lot better, but cannot get rid of the problem completely.

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Oct 15, 2017

Another important observation is that you will also notice that when the mat-select panel opens, the popover is not fired - you have to shift the mouse pointer a little bit to fire it.

Also, both mousenter and mouseleave have accessibility issues. The ideal events to trigger the popover should be the combination of the mouse events with focusin and focusout, which can be achieved by keyboard arrows (but without triggering twice the popover) [edited => in this case, I think there is an issue with mat-select, which doesn't emit anything nor give a clue about what is currently focused while you navigate throughout the options - maybe this is something that Kristiyan (crisbeto from Material team) should take care of].

This made me think whether making the hover (specifically) event an out-of-the-box feature isn't a good idea. Right now I haven't figured out a solution to fire the popover on select panel's open event (Should I have to code anything in mat-select's onOpen? what?). The panel is already opened, but it seems the mouseenter event was never emitted.

[edit => BTW, bootstraps's popover I'm currently using in my other project, also have the same behavior: until I move the mouse over the option, I don't see the popover]

@willshowell
Copy link
Contributor

First, I'm so happy to see that you're putting it through its paces so quickly! The feedback is wonderful!

Re: option templating, I think ng-container would solve everything here. StackBlitz. I may have missed something about the need to encapsulate in a separate component, but if you just anchor directly to mat-option, it should position correctly.

Re: dynamically creating and controlling popovers, between ngFor and ViewChildren, I think most cases should be covered. I'd like to get to snackbar-like usage so more logic can be encapsulated inside a popover, but I'm more concerned about being able to easily set a scroll strategy or focus behavior.. and those pesky tests...

Re: hover/focus events, I'll try to play around with it sometime this week and see what I can come up with. If there are no ways to determine which option is currently active, I agree, there should be.

Maybe you can combine elementFromPoint and the select's onOpen? Just brainstorming...

@julianobrasil
Copy link
Contributor Author

I had never used ng-container to isolate template variables before. Didn't know it was possible to do it. This is perfect. It's never too late to learn the obvious. 😄

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Oct 16, 2017

I've sent it to production. It'll be used for about 50 users (3 to 4 of them on a daily basis) and I'll keep an eye on it for eventual issues, but I honestly think that there will be any.

[edit]: "... honestly think that there WON'T be any"

@willshowell
Copy link
Contributor

Haha yeah, placing ngFor or ngIf on ng-container blew my mind when I realized it was possible. I dunno if it'll help either, but reassigning things inside ngIf is another neat trick, especially if you don't want to keep subscribing all over your template,

<ng-container *ngIf="user$ | async; let user;">
  <div class="name">{{ user.name }}</div>
  <img [src]="user.img">
</ng-container>

I think I will still remove the default click behavior in the short term. A longer term goal may be to have something like

<div [satAnchorFor]="myPopover" satTogglePopoverOn="hover focus longpress">
  ...
</div>

<div [satAnchorFor]="differentPopover" satTogglePopoverOn="focus click">
  ...
</div>

@julianobrasil
Copy link
Contributor Author

I think this would be nice because of some caveats, like disabling the backdrop on hover events.

In this case,what would be the default behavior:

  1. Nothing: sat-popover would throw an exception if satTogglePopoverOn wasn't explicitly set?
  2. Nothing: the popover simply isn't shown
  3. One of the pre-built events

@willshowell
Copy link
Contributor

Probably 2. I'm afraid though that it will get complicated to prioritize defaulted behavior if you have conflicting needs. As in,

  • How does focus work with a focus trap?
  • What if you want to open on mouseenter but then click to close (I've seen some discussions on /material2 about using mouseenter to quick-open menus)

A better solution may just be to briefly explain in the readme how to create a variety of behaviors, what the backdrop does, and StackBlitz demo(s) showing several common use cases. Now that you understand that the backdrop blocks hover events, it makes sense, right? I think if that were presented upfront in the README, it wouldn't have been so perplexing.

@julianobrasil
Copy link
Contributor Author

How does focus work with a focus trap?

😲 I haven't thought of that...

A better solution may just be to briefly explain in the readme how to create a variety of behaviors, what the backdrop does, and StackBlitz demo(s) showing several common use cases.

I agree.

@willshowell
Copy link
Contributor

@julianobrasil did you ever figure out a good way to get the onOpen event working to show immediately?

I think this issue can be closed unless I'm forgetting something. There is a blurb in the readme about how the backdrop can effect hover behavior, and it's now opt-in, which I think helps.

In the back of my mind, I'm considering extending the anchor in ways like [satTooltipAnchorFor]="tooltipPopover" and [satMenuAnchorFor]="menuPopover" where they have opinionated baked-in behavior. I figure the best option is to wait and see if people have issues with the current behavior, or find it annoying.

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Nov 6, 2017

did you ever figure out a good way to get the onOpen event working to show immediately?

No. I've noticed material tooltip has the same behavior. We are not alone 😄

In the back of my mind, I'm considering extending the anchor in ways like [satTooltipAnchorFor]="tooltipPopover" and [satMenuAnchorFor]="menuPopover"

You can be proud of your work. I wouldn't change it. It's so easy to use and so versatile at the same time. I doubt someone will ever complain.

Oh, and I haven't close this because I was not sure whether you were using it to track anything. IMO it can be closed. The onOpen thing is not directly related to satPopover. It's kind of a general js question.

@julianobrasil
Copy link
Contributor Author

BTW, I really liked that speed dial example in stackblitz.com.

@julianobrasil
Copy link
Contributor Author

julianobrasil commented Nov 19, 2017

Will, I've noticed today that the combination SatPopover + MatSelect is working just fine in the onOpen event (showing immediately when MatSelect's panel opens: we've been talking about it since #35 (comment)).

Have you done anything? Or have Material guys done?

[edited 1]: Or have Chrome guys done? I've checked out your old stackblitz example and it's working fine (Alpha.3).

[edited 2]: I think Chrome guys did something: the issue remains in Edge.

@willshowell
Copy link
Contributor

Good catch! It did not work in v61 but does in v62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants