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

Add alternative <dialog> element renderer #338

Merged
merged 13 commits into from May 25, 2018

Conversation

@timfish
Copy link
Contributor

commented Jan 22, 2018

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

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

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

@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

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 added 2 commits Jan 23, 2018
@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

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

@timfish timfish changed the title [WIP] Use <dialog> element Use <dialog> element Jan 27, 2018

@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

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

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

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

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

@timfish timfish changed the title Use <dialog> element Add alternative <dialog> element renderer Jan 29, 2018

@dannyBies

This comment has been minimized.

Copy link

commented Feb 7, 2018

Is this ready to be merged?

@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

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

This comment has been minimized.

Copy link

commented Feb 7, 2018

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

@StrahilKazlachev

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2018

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

@timfish

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2018

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

@Alexander-Taran

This comment has been minimized.

Copy link
Member

commented Apr 8, 2018

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

@CasiOo

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

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

This comment has been minimized.

Copy link

commented Apr 25, 2018

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

@dannyBies

This comment has been minimized.

Copy link

commented May 15, 2018

It would be really great to see this merged!

@StrahilKazlachev StrahilKazlachev merged commit f7d4c7a into aurelia:master May 25, 2018

2 checks passed

WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
bigopon added a commit to bigopon/aurelia-dialog that referenced this pull request May 11, 2019
@RomkeVdMeulen

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.