Add alternative <dialog> element renderer

[timfish]

I wont repeat everything I've already said in #335.

This would finally close #1 and improve accessabilty.

Breaking Changes

• startingZIndex setting is no longer required
• position callback is no longer passed overlay element as it no longer exists
• centerHorizontalOnly is not used. CSS only.

To do

• [x] Test that everything all works in polyfilled browsers
  • [x] Karma tests pass in Firefox 58.0 (2 transition tests still failing)
  • [x] Test real world usage in some other browsers

[timfish]

The transition* events don't appear to fire when the dialog is opened via showModal().

When manually adding and removing the open attribute the transition events appear to work. We can't however manually add/remove the open attribute as we would lose all the dialog goodness.

This seems to suggest animation is possible so we might just have to use another attribute to fire the transitions.

[StrahilKazlachev]

@timfish Ignore the transitions part, it will be gone soon.

[timfish]

The tests are still failing but I've managed to get transitions working in normal usage by having transition on .active rather than the open attribute and only calling <dialog>.close() after the transition is over.

@StrahilKazlachev do you know why centerHorizontalOnly is even required? Can't all positioning be via CSS or position callback?

[timfish]

All but 4 of the transition tests now pass in Firefox with the polyfill.

[timfish]

I've now tested this working on Chrome, Chrome Android, Firefox, Safari Mac, Safari iOS 11, Edge and IE11.

They only differ on dialog positioning and checking the browser support this is entirely down to the use of fit-content:

Chrome, Chrome Android, Safari Mac, Safari iOS 11 Vertically and horizontally centred

Firefox Horizontally centred at the top

Edge and IE11 Top left

Can we just go flex and remove centerHorizontalOnly?

[StrahilKazlachev]

I took my time to think on this and for now I don't like the <dialog> element. It feels incomplete, and troublesome. Still, since the community has interest in it I propose we add one more out-of-the-box Renderer - DialogRendererNative. Having 2 Aurelia renderers will also give us more insight on how to improve our Renderer and related APIs. @EisenbergEffect Your thoughts?

[timfish]

@StrahilKazlachev That's a good idea, I'm already using this renderer like:

    .plugin('aurelia-dialog', (config: DialogConfiguration) => {
      config.useDefaults();
      config.useRenderer(CustomDialogRenderer)
    })

[EisenbergEffect]

Go for it! 😊

Sent from my iPhone

On Jan 28, 2018, at 10:26 AM, Tim Fish notifications@github.com wrote:

@StrahilKazlachev That's a good idea, I'm already using this renderer like:

.plugin('aurelia-dialog', (config: DialogConfiguration) => {
  config.useDefaults();
  config.useRenderer(CustomDialogRenderer)
})

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[timfish]

@StrahilKazlachev I'll rework this into its own renderer and revert all the other changes.

[dannyBies]

Is this ready to be merged?

[timfish]

It doesn't touch any existing code so it's pretty safe to merge.

The docs will need some updating to show how to use the alternative renderer but it's probably worth it getting some testing in a beta first.

[jmezach]

@timfish I think @dannyBies and I would be more than happy to beta test this ;).

[StrahilKazlachev]

This is not a separate package - the new Renderer, so can't do beta since we are currently in RCish. What is still bugging me is how not to export the Renderers and still be able to set them. For now we don't want them to be used as base classes.

[timfish]

Sounds like the easiest thing to do for now would be to just host this renderer in another package? It just makes maintenance a bit of a pain.

For those wanting to test it now, if you're using TypeScript you can just copy dialog-renderer-native.ts into your project, fix the imports and use it like:

    .plugin('aurelia-dialog', (config: DialogConfiguration) => {
      config.useDefaults();
      config.useRenderer(DialogRendererNative)
    })

For JavaScript you'll have to convert it or build and use the dist output.

[StrahilKazlachev]

OK, so lets make this to 1 commit with the appropriate commit message and I'll get it in.

[timfish]

Does Github not have the option to squash into a single commit on merge?

[Alexander-Taran]

@timfish it does.. but commit message - i don't remember

[CasiOo]

Looks like a lot of code is copy-pasted from dialog-renderer.ts. Can't we just use the exported functions from dialog-renderer.ts instead of copy-pasting code around :)?

Rename DialogRendererNative to NativeDialogRenderer

[timfish]

@Alexander-Taran On the couple of private repos that I maintain it does offer a custom commit message on squash merge and nothing in the original fork/branch ends up in the master commit history

@CasiOo I'll rename and refactor out all the copy-paste

[Alexander-Taran]

@timfish I'd suggest then if you don't want to rebase / squash yourself just to provide a "final" commit message so it'd be easier to copy/paste it when merge/squash will happen (-:

[timfish]

@Alexander-Taran I'll rebase it.

Looking through the duplication between the two renderers I could reduce it by making NativeDialogRenderer extend DialogRenderer. This might complicate things as they further diverge?

[StrahilKazlachev]

@timfish correct about the complications, there is a lot of work to be done around the Renderers in my opinion. That's why I'm pleased to have a second out-of-the-box impl, even if currently it complicates some aspects. Lets leave it as is.

[timfish]

I did try rebasing but it seems beyond me this morning. If you can squash merge this with a commit message something like below, that would be great:

feat(dialog-renderer): Add native dialog renderer

[timfish]

Perhaps the docs could do with some details on using the new renderer?

import { NativeDialogRenderer } from 'aurelia-dialog';

aurelia.use.plugin('aurelia-dialog', (config: DialogConfiguration) => {
  config.useDefaults();
  config.useRenderer(NativeDialogRenderer)
})

It will also require custom CSS to stop it looking awful.

[StrahilKazlachev]

Will do later today. Actually the Renderer css is what is bugging me. Separating the current inlined css will be easy for the custom elements, but for the renderers I have no good idea.

[timfish]

Yeah, the CSS should probably be renderer specific and editing the inline string is fragile at best!

Ah, also worth noting that native dialog needs polyfilling still on many browsers.

[Alexander-Taran]

I'd leave styling of native to the end user.. so it would require some fiddling and discovering the need in polifil as well (-:

[rdemetrescu]

Wow, amazing work! I really would like to see it merged soon.

[dannyBies]

It would be really great to see this merged!

[RomkeVdMeulen]

@timfish If I understand correctly, to polyfill this I need to do something like this:

window.dialogPolyfill = await SystemJS.import("dialog-polyfill"); // or require() or import() or whatever

Did I get that right?

[timfish]

Its expecting window.dialogPolyfill to be set and the easiest way to achieve this is to just load dialog-polyfill.min.js into a script tag in your html.

[glen-84]

No documentation?

[Alexander-Taran]

@glen-84 there.. I created a pull request (-: https://github.com/aurelia/documentation/pull/486