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

feat: add 'before-fire-event' hook to helpers with side-effects #1183

Closed
wants to merge 1 commit into from

Conversation

shamrt
Copy link
Contributor

@shamrt shamrt commented Jan 12, 2022

A 'before-fire-event' hook is added to any helper that involves side-effects — i.e. scrolls or changes values in the DOM prior to firing a change event. These hooks allow better control of the DOM in cases where code needs to be executed after a helper affects the DOM as well as before an event is fired.

This PR follows from my conversation in #1182, and should serve to resolve an issue I have following @rwjblue's suggestion therein.

A `'before-fire-event'` hook is added to any helper that scrolls or changes values in the DOM. These hooks allow better control of the DOM in cases where code needs to be executed _after_ a helper affects the DOM as well as _before_ an event is fired.

return settled();
})
.then(() => runHooks('fillIn', 'before-fire-event', target, text))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that you would actually add this to fireEvent itself, that way you only haev to change one file (not all of hte dom helpers).

Is there any reason that wouldn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it'll work. Giving it a shot this morning 👍

Copy link
Contributor Author

@shamrt shamrt Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm now deep into this thing, and thought I should check in about my progress and confirm that the direction I'm heading is kosher.

Specifically, given the hooks system uses Promises, I've been forced to "Promisify" any function where fireEvent is called. Given the library forbids the use of async/await, in places, the changes are fairly substantial and it's not easy to keep things readable. I've managed to keep things passable.

My assumption about async/await is that we forbid it for backwards compatibility with older apps. Is that correct?

Anyway, I'm about 60–80% of the way done now.

@shamrt
Copy link
Contributor Author

shamrt commented Jan 14, 2022

Closing this PR in favour of #1185

@shamrt shamrt closed this Jan 14, 2022
@shamrt shamrt deleted the before-fire-event-hooks branch February 7, 2022 21:36
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

Successfully merging this pull request may close these issues.

None yet

2 participants