search

aurelia / ux

A user experience framework with higher-level capabilities, designed to bring simplicity and elegance to building cross-device, rich experiences.
MIT License
368 stars 55 forks source link

WIP: Drawer and ModalService #246

Closed ben-girardet closed 4 years ago

ben-girardet commented 4 years ago

@EisenbergEffect @bigopon @serifine

Here is a draft for the ux-drawer component and a ModalService.

At the moment I have focused the work towards a drawer component, but to be honest it works also as dialog. Basically the drawer can be set at position:

The ModalService is called as such because the intention is to use the same service across different components that would need to act as modal. This helps with keeping track of zIndex and properly layer components if several modals must be opened in a row. It is currently working only for drawers, but we can add support for other components as they come.

Drawers can be used either inline or with the ModalService. If they are used inline they don't need the service at all. I've set up a few examples in the dev app.

Inline use

<!-- Check out examples and code source for more -->

<ux-drawer if.bind="anyVarFromTheHostingVM">
  <!-- any content here that will be included in the slot -->
  <!-- I recommend using a ux-card as they fit well the purpose of this kind of surface -->
  <!-- BTW: I've set up a few CSS for when cards are used as drawer/dialog markup -->
</ux-drawer>

Service use

import { ModalService } from '@aurelia-ux/drawer';
import { inject } from 'aurelia-framework';
import { Hello } from './hello';

@inject(ModalService)
export class App {
  constructor(private modalService: ModalService) {}

  public openDrawer() {
    const drawer = this.modalService.open({viewModel: Hello, position: 'bottom', theme: {/*... */}});
    drawer.whenClosed().then((result) => {
      if (!result.wasCancelled) {
        console.log('output', result.output);
      }
    })
  }
}

For the ModalService I've tried to stay as close as possible to the current DialogService and DialogController api from ux-dialog plugin (except that the service handles both creating the drawer with open() and the cancel() and ok() apis from the included VM.

The .open() api accepts a viewModel, view or both (+ some more options). It returns a UxDrawer instance with an additional whenClosed() promise similar to current DialogService.

Known limitations

When using the ModalService, there are two limitations compared to the ux-dialog

  1. missing canActivate() lifecycle hook. Because I rely on <compose> to build the VM/V, then I can't can canActivate from the service and then compose do not do that either. If this would be an important feature then we should not rely on <compose> and create the composition from the service itself
  2. again, due to the <compose> usage, the included view cannot have any classes, attributes or event listener on the <template> tag.

This is a first draft that I want to publish here for early feedbacks.

I've setup my current showcase link as a demo of this PR: https://ux.mintello.com

Here is a quick gif to see the result:

Various Positions drawer1

Mobile drawer2

Go Crazy drawer3

EisenbergEffect commented 4 years ago

Thanks @ben-girardet This is looking nice. I admit to having a lot of fun playing with the demo :) Unless I missed it, I think we need to add some stuff around accessibility. That's the only thing that jumped out at me in the code. When playing with the demo, I did see some odd behavior when opening overlapping drawers. I didn't dig in to see what was going on though.

ben-girardet commented 4 years ago

Thanks @EisenbergEffect for your feedbacks.

Accessibility: did you mean keyboard support or did you mean something else ? I pushed a commit with keyboard support now.

Odd behavior: you're right, I've also discovered that if we open one or several "crazy" drawer and then close them... and after that we open an inline drawer => then there will be as many inline drawer elements attached to DOM than we previously opened "crazy" drawers.

I don't exactly know what is going on here. I understand that the issue comes because the crazy drawer attaches the same module again (containing the inline drawer) and it probably means that the inline drawer is somehow not properly detached when the module go away.

Looking forward to get more feedbacks. Especially about these questions:

EisenbergEffect commented 4 years ago

@ben-girardet I actually like the way you used compose and enhance together. Very clever and the code seems much cleaner than I remember some of the old dialog code being (though I haven't looked at that in a while).

In terms of accessibility, yes keyboard support, but also there may be some aria attributes we need to add to the backdrop and modal containers.

ben-girardet commented 4 years ago

Updated component to make it work even more like current ux-dialog plugin.

ben-girardet commented 4 years ago

I've added this paragraph to the PR description:

Known limitations

When using the ModalService, there are two limitations compared to the ux-dialog

  1. missing canActivate() lifecycle hook. Because I rely on <compose> to build the VM/V, then I can't can canActivate from the service and then compose do not do that either. If this would be an important feature then we should not rely on <compose> and create the composition from the service itself
  2. again, due to the <compose> usage, the included view cannot have any classes, attributes or event listener on the <template> tag.
ben-girardet commented 4 years ago

Quick note to say that I think it is valuable to explore how to call the canActivate hook.

It is an important piece in the VM lifecycle. It is the only way to give users a chance to validate if the VM receives the right model to create and handle the modal.

ben-girardet commented 4 years ago

Thanks a lot @bigopon for your first round review.

I left a few comments. I am particularly concerned about the discussion on detached(). From my tests there is something not going quite right when detaching Drawers created by the ModalService.

For the record:

This is the code that insert the drawer:

options.host.appendChild(element);
let childView = this.templatingEngine.enhance({ element: element, bindingContext: bindingContext });

Now the question is: what's the right way to do the exact opposite (with full respect of Aurelia Lifecycle).

My current approach (could well be wrong):

const controllers = (childView as any).controllers as Controller[];
const drawer: UxDrawer =  controllers[0].viewModel as UxDrawer;

drawer.detached();
const parent = drawer.element.parentNode;
if (!parent) {
  return;
}
parent.removeChild(drawer.element);
ben-girardet commented 4 years ago

@EisenbergEffect @bigopon

I think I have some good news. I have refactored a little the service in order to add the canActivate() method in the attached VM lifecycle (if any). To be honest I took quite a few parts from the dialog plugin codebase to do so.

This is very useful as a dialog or drawer might not be able to work properly if it doesn't receive the proper params in the activate() method. Therefore being able to check this earlier with the standard Aurelia lifecycle method canActivate() seems a non-negotiable.

The good news that comes with it is that it fixes the previous bug discovered here. So now it seems that anything attached with the service is properly detached and do not interact negatively anymore.

There is however a bad news. And I'm seeking help here. If you try the demo now and open a crazy drawer (meaning that it opens the current view) you will discover that the chips choices don't work anymore. This is very weird as everything else works fine (checkbox, buttons, ....). I wonder where this could come from. Could it be that the custom attributes (ux-choice-container) are not processed correctly with this new composition method ? Any ideas ?

As always: I'm open for any feedbacks

EisenbergEffect commented 4 years ago

I did a quick inspection of the DOM and the ux-choice attribute is still present, but with wildly different values, so it appears that something is definitely wrong with that attribute. I wouldn't expect to see it in the DOM at all, since these attributes are removed during compilation (unless I'm getting versions confused in my head).

ben-girardet commented 4 years ago

I followed on your findings @EisenbergEffect

The ux-choice-container and ux-choice attributes are still present in the DOM for both the working and non-working attributes. I can't tell if they are supposed to stay in the DOM or not for au1 ?

However there is something wrong in the attributes lifecycle when they are part of the dynamic composition. I could identify that for the working attributes (part of the main page) the attached and detached methods are called. They are not called when the attributes are part of the dynamic composition.

I'm thinking the problem can come from how I set up the compositionContext, especially regarding either the container, childContainer or bindingContext value. At the moment as a container I pass the container value of the childView created by the previous enhance on the drawer. Not sure if this is correct ?

  let childView = this.templatingEngine.enhance({ element: element, bindingContext: bindingContext });

    let slot: ViewSlot;
    const controllers = (childView as any).controllers as Controller[];
    const drawer: UxDrawer =  controllers[0].viewModel as UxDrawer;
    try {
      const view: any = controllers[0].view;
      slot = new ViewSlot(view.slots['__au-default-slot-key__'].anchor, false);
    } catch (_error) {
      this.cancelOpening(drawer);
      throw 'Missing slot in drawer';
    }

    let compositionContext = this.createCompositionContext(childView.container as any, element, bindingContext, {
      viewModel: options.viewModel,
      view: options.view,
      model: options.model
    }, slot);
  private createCompositionContext(
    container: any, 
    host: Element, 
    bindingContext: ModalBindingContext, 
    settings: {model?: any, view?: any, viewModel?: any},
    slot? : ViewSlot
    ): CompositionContext {
    return {
      container,
      bindingContext: settings.viewModel ? null : bindingContext,
      viewResources: null as any,
      model: settings.model,
      view: settings.view,
      viewModel: settings.viewModel,
      viewSlot: slot || new ViewSlot(host, true),
      host
    };
  }
ben-girardet commented 4 years ago

@EisenbergEffect @bigopon

I would greatly appreciate some help to find out what's wrong with the composition in my code. In order to help debugging I added some console.log that you can see in the demo.

Basically I console.log each attached/detached of ux-choice-attribute with an incremental index on each to see when they are properly attaching/detaching or not. Going in and out of the drawer page works fine, also opening an inline drawer, but not with the service using the composition.

I've logged a few steps in the composition process as well.

If you have any ideas for fixing this that would be sooo awesome!!

EisenbergEffect commented 4 years ago

@bigopon has more recent experience working inside the templating engine and composition pies. @bigopon Any ideas what might be going on here?

bigopon commented 4 years ago

@ben-girardet from your demo, it seems attached hasn't been called for the view model of ux-choice-container: image hence why click events are not registered. Another thing I noted during debugging this issue is that in our components, we are using click.delegate. I think for plugin, we should be using click.trigger instead, as any code above the component calling stopPropagation may potentially affect our handler. Also, in terms of encapsulation, .trigger feels better

ben-girardet commented 4 years ago

Thanks @bigopon for your valuable feedbacks. Always good to learn from you!

The composition still fails to properly load the custom attributes :-(

I have created a Dumber Gist and a small git repro for this issue... Maybe easier to spot what's wrong from one of them..

bigopon commented 4 years ago

@ben-girardet thanks for the gist, i'll debug it

bigopon commented 4 years ago

@ben-girardet The behavior you saw is because .compose on composition engine only bind and add a view to a view slot, but does not try to call attached() on the controller created. The job of calling attached method is delegated to the view slot. So you only need to call attached() for our view slot and any views added to it will be invoking attached too.

Here's and example: https://gist.dumber.app/?gist=3f3d935b7bc872b8eb59651f8c211620&open=src%2Fapp.ts

ben-girardet commented 4 years ago

Thank you so much @bigopon

This is really helping me!! Looking forward to move forward with this PR.

ben-girardet commented 4 years ago

@EisenbergEffect @bigopon

Here is an important update on the drawer component.

New: modal as contextual menu

My vision when starting this drawer component was to create something that works for all kind of situations. I've used the contextual menu situation as a proof of concept. Using the new openingCallback() and the position = 'absolute' in the modal config it was pretty easy to come up with a function to position the modal next to an anchor.

I've now added a positionModalWithAnchor() method in the modal service that do the calculation. Here is the code to open a ViewModel as a menu:

this.modalService.open({
  viewModel: Hello,
  position: 'absolute',
  openingCallback: (contentWrapperElement: HTMLElement, _overlayElement: HTMLElement) => {
    this.modalService.positionModalWithAnchor(
      anchorElement, 
      contentWrapperElement, 
      {preferedPosition: 'bottom'});
  },
});

I would love to hear what you think about all this. Hopefully this is going in a direction that makes sense to you as much as for me :-/

Demo

As always: https://ux.mintello.com/#/modal (scroll down for the menu demo)

bigopon commented 4 years ago

@ben-girardet I haven't reviewed the code, and while playing with the demo, noticed that after opening drawer, menu positioning stops working

ben-girardet commented 4 years ago

@bigopon could you give more details on what doesn't work.

Or could it be that when you tried the menu positioning you didn't have the anchors on the screen and it seemed that the positioning didn't work ?

Capture d’écran 2020-04-03 à 07 29 28
ben-girardet commented 4 years ago

@bigopon wondering if you still experience the positioning issue ?

bigopon commented 4 years ago

I'm getting this issue: modal_drawer_anchor_err

The first modal trigger button couldn't locate the anchor correctly after open the drawer

ben-girardet commented 4 years ago

Hum... are you using Safari @bigopon ? There seem to be something bugging in Safari desktop...

ben-girardet commented 4 years ago

The issue that you experience @bigopon comes from a reference issue with the view-model of the drawer. What is happening is

Now comes my questions:

ben-girardet commented 4 years ago

@bigopon nothing wrong with composition, only something wrong with my code :-/

Fixed: https://github.com/aurelia/ux/pull/246/commits/0991be48cb3a05e663ec45f2eb19a3a6ca126159

Do you have any comments regarding the current state of this PR ?

If none, this is my plan:

ben-girardet commented 4 years ago

@bigopon @EisenbergEffect

I've worked on the positioning service. I got inspired by the popper library and in order to avoid dependency I've built something from scratch. I turns out working pretty well. I've already included some uses cases such as:

It can surely be optimized. But first I wanted to try and have a proof of concept. Open for feedbacks.

Commit: https://github.com/aurelia/ux/pull/246/commits/1b65e4d4e94ad7a2dd64a5a8aed21c5ca662607d

Demo: https://ux.mintello.com/#/anchor

EisenbergEffect commented 4 years ago

@ben-girardet Awesome! Thanks for continuing to keep this moving ahead. I seem to have an issue accessing the demo link. Can you confirm that it's correct and working?

bigopon commented 4 years ago

@ben-girardet thanks, very nice work. For review purposes, can you help remove all of the dist files? We normally leave it to build process during the release

ben-girardet commented 4 years ago

@EisenbergEffect ouch sorry server is down. will fix asap. might take another hour :-/

@bigopon sure, my mistake, normally I leave them out of commits. I've removed them

ben-girardet commented 4 years ago

Quick update: I have now finished the ground work for the positioning service and I can use it for positioning modals (not just demo elements).

So I've updated the demo which now use the positioning service on the demo page. Happy to see how it turns out.

https://ux.mintello.com/#/modal

Happy easter everyone !

bigopon commented 4 years ago

@ben-girardet very nice. <ux-select/> modal can also be used as a test subject. And in long term, we would want it that way too

ben-girardet commented 4 years ago

@bigopon that would be fantastic! A good real world test!

Would you be willing to team up with me for this? I feel a little lost in the way the select is architectured.

In order to use the modal the best would be to have a separate VM/V for the options list that should be in the modal. That’s where I need help.

What would be the steps to make that happen?

The only requirement of the VM modal is to parse the params in the activate hook to generate its content. So if we can pass on an array of options we should be able to build the view.

Also the benefit of using a VM/V for this modal is that we could make it configurable so that users could even change the look. Maybe...

bigopon commented 4 years ago

@ben-girardet the general theme of <ux-select/> is to mimic native <select/>, as close, as possible. We will be providing a 2nd component, probably named something close to a "combobox" that has:

For the <ux-select/> itself, it's mainly this line https://github.com/aurelia/ux/blob/fb4118a38b04c99fe2a74678162d77f1752fbd4f/packages/select/src/ux-select.html#L30 at the moment. You can see that wrapper will be set displayed fixed and show/hide accordingly, with some animation. But probably, that's not gonna work properly inside a fixed container. About the container of the option list, there are 2 <div/>s, 1 is to position the list relative to anchor, and to show/hide. Another one, inside that, is to take care of the animation.

I'm not sure about how we want to deal with it eventually: (1) only positioning service - extracted & separated from modal, or (2) use the entire modal. For (2), I'd imagine that it'd be a big change in the code, and I'm not sure yet, as atm, we walk the DOM to retrieve stuff I believe. Maybe need to use container to inject stuff now

ben-girardet commented 4 years ago

Thanks for your explanation. I also hesitate between 1) and 2).

1) Using only positioning should be pretty easy. As we don’t have « modal » stuff with it we need to handle the « click outside » to dismiss the wrapper. I think it’s already done btw.

2) Using modal we could provide still a few options to the user such as « lock » or a full screen mode in mobile. As the modal also work inline we can use it to replace the actual wrapper. It would be a bit more work though because then we need to rewrite the open/close/dismiss things.

ben-girardet commented 4 years ago

I've given a quick try for the option 1) It's working so so at the moment.

I feel like I can continue with using only the positioning service but I might need to use the same "trick" than for the modal and move the wrapper as child of the <body> before positioning.

This tend to avoid problems if the select is placed inside a fixed container and has the benefit to not alter the width and height of the ux-select element itself (we need correct width and height for the ux-select as it acts as an anchor).

If you think this is too much, I can also wrap the select part inside a div and then use this div as anchor to have correct width and height. The benefit here is that I don't need to move the <slot> container as a child of body.

Option 2) (using modal) would be cool as it already resolves all these issues and create a consistent feel in the UX lib. But here I face an issue with the <slot>.

If I would use the modal component inline, the main idea is to use it with an if.bind for opening and closing. It's pretty neat because then it doesn't polute the DOM with elements that are not required when modal is closed. But then the <slot> doesn't work because it cannot be placed inside a if.bind.

Did you already faced similar situations with <slot> used behind a if.bind ? Do you have some ideas how to fix this ?

Personally I've already tried something in a component where I placed the <slot> in a fake container (hidden). Then I needed to watch this container and ref all its children so that I could move then at the right place when the if.bind becomes true.

But I feel that this solution is not very stable and would prefer to have something more robust if we go in this direction.

ben-girardet commented 4 years ago

@bigopon I've sticked to the modal positioning service and I'm happy with the result. Took me a bit of time to get my head around this select but in facts it's not so complicated...

Please have a look a the commit and the demo and tell me what you think.

Note: I've disabled the collapse on wheel now that I can easily update the position of the modal on scroll. It gives a nice effect when you scroll and don't have enough space in the bottom => then the list moves towards the top of the select.

bigopon commented 4 years ago

@ben-girardet that looks nice, thanks for doing that. I noticed one difference is that the list modal is positioned at the bottom of the select box, while with material design, I believe it's should be at the start? Also, there's one detail it doesn't have yet: the modal should be opened with selected option align well on the select box. If this feels like off topic of this PR too much, we can defer the select improvement until later. As we need more feature in positioning like offset, maybe?

ben-girardet commented 4 years ago

We can change the placement by setting top-start instead of bottom-start when setting the positioning instance.

As for the precise positioning could you picture what you mean? The positioning service already have the offset setting ready so it might me a matter of setting it correctly.

By default there is a 5px offset between anchor and modal. Do you mean this should be 0 ?

ben-girardet commented 4 years ago

Do you refer to https://material.io/components/menus/ for the specs on select ?

What I understand is that it should have a 0 offset so that the list stick to the select.

Regarding positioning it all depends on screen real estate around the anchor. We have the flipping feature that does a pretty* good job. I didn't find something saying that it should defaults to top. Seems that defaulting to bottom is fine, as long as there is enough space.

* The difference I noted from their examples is that when the modal is displayed on top of the select, it should cover the select instead of being placed right on top. However I don't quite like this behavior as it mask some context of the field. But I'm open to add a setting in the positioning service. That would look like setting a flippingStrategy config to tell if the flip must cover the anchor or not

bigopon commented 4 years ago

@ben-girardet thanks for the reference link. From there: image

It seems we can start with the exposed menu style, which is more similar to native select. If folks insists, we can go with dropdown menu style. I originally went with this, but I'm not entirely sure about it being the default behavior

bigopon commented 4 years ago

For

Do you refer to https://material.io/components/menus/ for the specs on select ?

Yes, I originally based the implementation on some other component libs, thinking that is the default, but clearly not necessary.

ben-girardet commented 4 years ago

@bigopon the way I read this is:

So for the select we should use exposed scenario. The dropdown will refer to the menu component (I hope we could continue the work you started).

bigopon commented 4 years ago

@ben-girardet yes let's go with that. Dropdown style for the <ux-menu/> sounds valid to me as well 👍

ben-girardet commented 4 years ago

I've updated the positioning service with the following

Missing Space Strategy

Note: this strategy is currently only applied to the main placement. It means if we request a top-start placement, it will be applied to the top but not (yet) the start part.

Demo

https://ux.mintello.com/#/anchor

ben-girardet commented 4 years ago

I've implemented the ignore strategy for secondary placement. The other ones are not relevant in my opinion.

ben-girardet commented 4 years ago

I ping @EisenbergEffect and @bigopon here because I believe the positioning service is ready for review. I have some questions below, but first, in a nutshell, here is what this service does:

(*) The reasons why I chose (for now) not to handle scroll or resize is that I prefer to leave the positioning focused on what it's supposed to achieve. Listening to such events and update automatically could be nice but in order to cover each edge case it could end up being much work.

Positioning Options

UxModalPlacement

A few questions for the review

  1. Package: As this service is autonomous, I don't think it should stay in the modal package. I am tempted to create a positioning package inside the learn mono-repo and have this service stand alone. The benefit is that this service could be used without any of the rest of the library if this is what someone wants. Also, it will be useful in other packages, such as the select component and the rest of the modal component is not required.
  2. Naming: I've noticed that Aurelia doesn't have much classes called ....Service but rather ...Engine. Maybe this would be better called UxPositioningEngine
  3. Factory: In order to work well with the DI (because I need to inject TaskQueue), I've created a single class that have a getInstance() public method to create an instance correctly linked to the DI. But I believe there are better patterns as Factory for this kind of things. Do you have any advice to make this better ?
bigopon commented 4 years ago

For naming, probably @EisenbergEffect can tell, I'm leaning towards engine as well. We do have DialogService, and this is gonna probably be ModalService as well, but PositioningEngine sounds closer to StyleEngine

For packaging, I'm ok with having a separate package for it, though it's not my call 😁 @EisenbergEffect . Maybe we can invite some people to try it out in their non-Aurelia projects and help spot bugs.

What is the last point about?

EisenbergEffect commented 4 years ago

Pulling it out into its own package seems fine. Not sure about "engine" or "service" actually. You could just call it "ElementPositioner".

ben-girardet commented 4 years ago

Wow thanks for this great review ! I'll look in detail at each comments.

ben-girardet commented 4 years ago

Thanks again @bigopon for all your valuable comments. I've replied / fixed them.

I was not clear, but the current drawer package is out of date as everything is currently in the modal package. But don't worry, I have applied all your comments directly to the modal package.

From now on, in order to bring a little more clarity on this PR I suggest to create two distinct PRs and close this one as WIP without merging.

1. Positioning PR With a new positioning package containing only the positioning service.

I tend to prefer the PositioningService or PositioningEngine names which seem to follow a little more the other naming in Aurelia codebase. But of course I leave the last call to @EisenbergEffect

2. Modal PR With the modal package containing the current implementation of the component and service. I'm planning to finish up a few more things before final review.

Would you agree with this course of action ? If so we could probably merge the positioning PR pretty soon.