✨ <Dialog/> feature/support for nesting dialogs
#64
Conversation
| import { hbs } from 'ember-cli-htmlbars'; | ||
| import userEvent from '@testing-library/user-event'; |
There was a problem hiding this comment.
As triggerEvent and triggerKeyEvent don't simulate changing focus element - we need some emulator.
In react they created own script:
link
There was a problem hiding this comment.
I've not used @testing-library. This is preferable over re-implementing the same code that you linked to in the headless react lib?
There was a problem hiding this comment.
Not sure, but regarding that is only dev dependency, I use it
There was a problem hiding this comment.
This does feel a little odd to me too, though I honestly can't determine if it's actually is weird or if it's just not a pattern that I've seen before. These packages are meant to be framework agnostic, so it should be fine, I think?
Maybe it makes more sense to borrow that helper function from the React implementation and use that ourselves, and avoid the extra testing packages?
There was a problem hiding this comment.
It should work with any framework.
About copying and pasting from react, regarding that they use jest there are some framework specific code:
link
so copy and paste will not work in our work.
far-fetched
force-pushed
the
feature/support-for-nesting-dialogs
branch
from
294c02a to
ecc81ad
Compare
There was a problem hiding this comment.
Oh, I love this. Feels like the stack service implementation is a good fit.
Love your work. Thanks @far-fetched .
@GavinJoyce @alexlafroscia Fancy giving this a looksee before I merge and ship it?
|
@far-fetched Obviously the broken test run needs fixing. Node version update needed by the looks of it. |
far-fetched
force-pushed
the
feature/support-for-nesting-dialogs
branch
6 times, most recently
from
c69b500 to
36d7fc4
Compare
| 'it should be possible to open nested Dialog components and close them with `Escape`', | ||
| async function () {} | ||
| ); | ||
| test.each( |
There was a problem hiding this comment.
Nice use of test.each here!
| node-version: 10.x | ||
| node-version: 14.17.2 |
There was a problem hiding this comment.
What made this required? The @testing-library packages?
There was a problem hiding this comment.
Yep, then I updated it to last LTS version.
addon/services/stack-provider.js
Outdated
| stack = []; | ||
|
|
||
| @action | ||
| hasOpenChild(dialog) { |
There was a problem hiding this comment.
This is checking that the dialog argument is the currently-active dialog, yeah?
The name of this method is throwing me off a little -- it sort of suggests that it's checking if the dialog argument has a child that is open, rather than that the dialog argument is itself open.
There was a problem hiding this comment.
Hmm I think that this method does exactly what you said:
it sort of suggests that it's checking if the
dialogargument has a child that is open
if a dialog has an active (opened) child - which is the last element added to the stack, it will return false, otherwise true;
There was a problem hiding this comment.
Maybe turning it around to be something more like: isCurrentlyOpenDialog which checks that the provided dialog uid is the last one in the stack? This might be less cognitive load.
There was a problem hiding this comment.
hmm this is the oposit logic, so it will change:
if (this.dialogStackProvider.isCurrentlyOpenDialog(this)) {
this.args.onClose();
}But If 3 dialogs are open (nested dialogs) ? The name of this method does not make sense, right ? I don't fully get what is wrong with hasOpenChild 🤔
There was a problem hiding this comment.
My confusion with hasOpenChild is that is suggests that the dialog argument passed to it is not itself the top-most dialog. It reads to me like the method is checking that the dialog being passed in itself has some child dialog, which is open, but I don't think that's the intention.
It might be more clear to adopting the naming of typical Stack operations: push, pop and peek. Then if you wanted to check if a given dialog is the top-most one, you can do this instead:
if (this.dialogStackProvider.peek() === this) {
this.args.onClose()
}The benefit here is that there's no longer ambiguity around what the intention of the code is; peek returns the top of a Stack, and we only want to operate if we're working with the Dialog at the top of the stack. The service then also conceptually makes to being just a stack without really needing any domain-specific behaviors, which further simplifies things. We can avoid getting bogged down by naming (which is hard) and lean on the conventions of the data structure we're using here.
After re-reading the code, I realize I've been misunderstanding all along; the method name does match the behavior. I'm sorry for the confusion!
I still do think that it could make sense to adopt the Stack-API-based naming for the methods on this service that is conceptually a Stack, but you're right @far-fetched -- the initial confusion that I had was my own fault.
| import { hbs } from 'ember-cli-htmlbars'; | ||
| import userEvent from '@testing-library/user-event'; |
There was a problem hiding this comment.
This does feel a little odd to me too, though I honestly can't determine if it's actually is weird or if it's just not a pattern that I've seen before. These packages are meant to be framework agnostic, so it should be fine, I think?
Maybe it makes more sense to borrow that helper function from the React implementation and use that ourselves, and avoid the extra testing packages?
far-fetched
force-pushed
the
feature/support-for-nesting-dialogs
branch
from
36d7fc4 to
0bed280
Compare
Fixes #63