fix: setFocus methods should wait for the component to be loaded #5749
Conversation
driskull
changed the title
driskull
changed the title
There was a problem hiding this comment.
🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶
🕶😎🕶🕶🕶🕶😎😎🕶🕶🕶😎😎🕶🕶😎🕶🕶😎🕶😎😎😎🕶😎🕶🕶🕶😎🕶😎😎😎😎🕶
🕶😎🕶🕶🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶😎🕶🕶🕶😎🕶🕶😎😎🕶🕶😎🕶😎🕶🕶🕶🕶
🕶😎🕶🕶🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎😎🕶🕶🕶🕶😎🕶🕶😎🕶😎🕶😎🕶😎🕶😎😎🕶
🕶😎🕶🕶🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶😎🕶🕶🕶😎🕶🕶😎🕶🕶😎😎🕶😎🕶🕶😎🕶
🕶😎😎😎🕶🕶😎😎🕶🕶🕶😎😎🕶🕶😎🕶🕶😎🕶😎😎😎🕶😎🕶🕶🕶😎🕶😎😎😎😎🕶
🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶
🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶
🕶😎😎😎😎🕶🕶😎😎🕶🕶🕶😎😎🕶🕶😎😎😎🕶🕶😎🕶
🕶😎🕶🕶🕶🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶
🕶😎🕶😎😎🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶
🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶🕶😎🕶😎🕶🕶😎🕶🕶🕶
🕶😎😎😎😎🕶🕶😎😎🕶🕶🕶😎😎🕶🕶😎😎😎🕶🕶😎🕶
🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶🕶
github-actions
bot
added
the
bug
# Conflicts: # src/components/filter/filter.tsx # src/components/pick-list/pick-list.tsx
driskull
changed the title
driskull
changed the title
driskull
added
the
pr ready for visual snapshots
There was a problem hiding this comment.
👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀
👀👓👓👓👓👀👀👓👓👀👀👀👓👓👓👀👓👀👀👀👓👀👀👓👓👓👀👓👀
👀👓👀👀👀👀👓👀👀👓👀👓👀👀👀👀👓👀👀👀👓👀👓👀👀👀👀👓👀
👀👓👓👓👀👀👓👀👀👓👀👓👀👀👀👀👓👀👀👀👓👀👀👓👓👀👀👓👀
👀👓👀👀👀👀👓👀👀👓👀👓👀👀👀👀👓👀👀👀👓👀👀👀👀👓👀👀👀
👀👓👀👀👀👀👀👓👓👀👀👀👓👓👓👀👀👓👓👓👀👀👓👓👓👀👀👓👀
👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀👀
Awesome!
WRT components using setFocus, as long as we have a follow-up issue and convention note to ensure we use the loadable util consistently, I think it's fine for the focusable test helper to not cover it since it's a Stencil lifecycle bug. It'd be really complex to test it consistently and would be out of scope regarding our test coverage.
src/utils/loadable.spec.ts
Outdated
| loaded: false, | ||
| promise: null | ||
| }; | ||
| const fakeComponent: any = { |
There was a problem hiding this comment.
I really like how this test is showing how the modules work together. If not too complex, we could do the same for other spec tests.
From that perspective, I think we could simplify the test and have it focus on its use case:
const afterDidLoad = fakeComponent.resolvesAfterDidLoad();
// assert afterDidLoad not resolved
fakeComponent.componentWillLoad();
// assert afterDidLoad not resolved
fakeComponent.componentDidLoad();
// assert afterDidLoad resolvedAlternatively, the individual utils could be tested as well:
// assert componentLoaded(fakeComponent) not resolved
setUpLoadableComponent(fakeComponent);
// assert componentLoaded(fakeComponent) not resolved
componentLoaded(fakeComponent);
// assert componentLoaded(fakeComponent) resolvedThere was a problem hiding this comment.
I really like how this test is showing how the modules work together. If not too complex, we could do the same for other spec tests.
Can we get a followup issue for this? 👍
// assert afterDidLoad not resolved
How can I assert the promise is not resolved? There's not a property on the promise. Is there some util for this?
There was a problem hiding this comment.
Here's one, untested, idea:
const afterLoad = jest.fn();
const load = componentLoaded(fakeComponent).then(afterLoad);
await waitForAnimationFrame();
assert(afterLoad).not.toHaveBeenCalled();
setUpLoadableComponent(fakeComponent);
await waitForAnimationFrame();
assert(afterLoad).not.toHaveBeenCalled();
componentLoaded(fakeComponent);
await waitForAnimationFrame();
assert(afterLoad).toHaveBeenCalled();# Conflicts: # src/components/popover/popover.tsx
|
@jcfranco this one good to merge? |
driskull
added
pr ready for visual snapshots
fix: setFocus methods should wait for the component to be loaded