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: document user-event v14 #980

Merged
merged 19 commits into from
Jan 10, 2022
Merged

feat: document user-event v14 #980

merged 19 commits into from
Jan 10, 2022

Conversation

ph-fritsche
Copy link
Member

Add documentation for user-event@14 alongside v13 for rest of beta.

New projects / projects that someone is actively working on should probably already switch to v14-0.0-beta, but I want to wait for more feedback before declaring this stable.
I hope the new documentation helps with this.

@MatanBobi
Copy link
Member

Hi @ph-fritsche! Thanks for tagging me to review this one :)
I'll have a look at it later on today but just to raise a discussion:
user-event is an important library in the testing-library family, I think it should get a bigger place in the docs and not just in the eco-system section, especially given the amount of docs you've just added.
Any thoughts?

@ph-fritsche
Copy link
Member Author

You won't see me arguing against a more prominent advertisement for user-event.

I think the change between v12.8.3 which still has >500k npm downloads per week and v14 is huge - not only if you measure it in code lines changed.
There is currently no bug issue left and the more fundamental approach that was necessary to implement some features (and some bug fixes) without endless adding of conditionals also made the codebase more maintainable.
I'd say user-event@14 is a very mature tool that I would recommend anywhere.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Nice! 🤩

docs/user-event/api-pointer.mdx Outdated Show resolved Hide resolved
Comment on lines +17 to +21
> Our primary target audience tests per `jest` in a `jsdom` environment and there
is no layout in `jsdom`. This means that different from your browser the
elements don't exist in a specific position, layer and size.
We don't try to determine if the pointer action you describe is possible at that
position in your layout.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean this?

Suggested change
> Our primary target audience tests per `jest` in a `jsdom` environment and there
is no layout in `jsdom`. This means that different from your browser the
elements don't exist in a specific position, layer and size.
We don't try to determine if the pointer action you describe is possible at that
position in your layout.
> Our primary target audience tests per `jest` in a `jsdom` environment and there
is no layout in `jsdom`. This means that the elements specific position, layer, and size could be different from your browser.
We don't try to determine if the pointer action you describe is possible at that
position in your layout.

Copy link
Member Author

Choose a reason for hiding this comment

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

No this literally means that there is no specific position, layer, or size. Elements can have IDL properties and/or CSS styles that usually correspond to elements being at a specific position, having specific dimensions, or being above/under each other. But jsdom has no layout. The properties and styles exist, but they have no effect.

document.body.innerHTML=`<img style="position: relative; left: 50px;"/>`
$("img").style.left // '50px'
$("img").offsetLeft // 0

Copy link
Member

Choose a reason for hiding this comment

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

👍 Got it, the comment made it click to me - would it be worth it to also mention it in the docs?

every character has effective width zero and is at effective offset 0,0. (While there might be properties applied that suggest otherwise but have no effect.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. I feel like it might be better to write a blog post about this topic explaining the concept and then adding a link. Really explaining this, and what difference it makes to testing might be worth it, but is out of scope for these docs.

docs/user-event/api-pointer.mdx Show resolved Hide resolved
docs/user-event/api-utility.mdx Show resolved Hide resolved
docs/user-event/api-utility.mdx Show resolved Hide resolved
docs/user-event/install.mdx Show resolved Hide resolved
docs/user-event/install.mdx Show resolved Hide resolved
sidebars.js Outdated Show resolved Hide resolved
docs/user-event/options.mdx Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Copy link
Member

@MatanBobi MatanBobi left a comment

Choose a reason for hiding this comment

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

This is amazing!
I left a few comments :)

docs/user-event/api-keyboard.mdx Outdated Show resolved Hide resolved
docs/user-event/api-clipboard.mdx Show resolved Hide resolved
title: Convenience APIs
---

The following APIs are shortcuts for equivalent calls to the underlying
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure these API's should be "hidden" in the convenience section because most of the users will probably be using them.
Either we should think of a way to rename them, change the order of the docs so they'll be first or mention them in the pointer and keyboard pages.

Copy link
Member Author

@ph-fritsche ph-fritsche Jan 6, 2022

Choose a reason for hiding this comment

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

Yeah, I don't know. On one hand they exist because they are easy to use and so - yes - they are probably used often, but on the other hand they are self-explanatory and merely documented for sake of completeness. The auto-completion in IDE would probably be enough.

Maybe this could be solved by another blog post walking through some examples with heavy use of these convenience methods and mentioning the post with a short excerpt in the introduction.
A mention in pointer and keyboard might also be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should rely on the IDE auto-completion for this one and IMO the package's functionality should be easily found in the docs and not in a personal blog post (but we should also think of writing a blog post about user-event 14 in the testing-library blog).

I strongly prefer a mention in pointer and keyboard pages :)

docs/user-event/api-pointer.mdx Outdated Show resolved Hide resolved
docs/user-event/api-pointer.mdx Outdated Show resolved Hide resolved
docs/user-event/api-utility.mdx Show resolved Hide resolved
docs/user-event/install.mdx Show resolved Hide resolved
docs/user-event/install.mdx Show resolved Hide resolved
docs/user-event/install.mdx Show resolved Hide resolved

```js
test('trigger some awesome feature when clicking the button', async () => {
const user = userEvent.setup()
Copy link
Member

Choose a reason for hiding this comment

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

This is only my opinion, but is there a way to setup userEvent in the setupFilesAfterEnv in jest for example?
I think causing our users to write this line in every test might cause some pain.

Copy link
Member Author

@ph-fritsche ph-fritsche Jan 6, 2022

Choose a reason for hiding this comment

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

Yes, this could be done in a beforeEach. But we especially mention Avoid nesting when you're testing in the docs...

The solution for now would be:

function setup(jsx) {
  render(jsx)
  const user = userEvent.setup()
  return { user }
}

test('something', () => {
  const { user } = setup(<MyComponent/>)
  // ...
})

This pattern might even become that common that we should consider returning the user-event instance from render in the framework packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an example for a test setup function below.

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of returning the user-event instance from render.
But this means people will need to install user-event too and the frameworks will instantiate that for them?
That sounds too "voodoo" to me.

Copy link
Member Author

@ph-fritsche ph-fritsche Jan 10, 2022

Choose a reason for hiding this comment

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

Yes, something like that.

The framework wrapper could try..require user-event. So that if user-event is installed the wrapper will automatically return an instance from render.

Or we could take a similar approach like how @testing-library/react configures /dom when it's imported.
user-event could register itself on the framework wrapper when being imported.

It might be a bit "voodoo", but it might also result in a good developer experience.

@ph-fritsche ph-fritsche linked an issue Jan 7, 2022 that may be closed by this pull request
Copy link
Member

@MatanBobi MatanBobi left a comment

Choose a reason for hiding this comment

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

This looks good enough for me to push forwards and maybe iterate later if you want :)

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

I've browsed through it for a second time, and it's also looking good to me 👍

@timdeschryver timdeschryver merged commit f30e5e8 into main Jan 10, 2022
@timdeschryver timdeschryver deleted the userevent branch January 10, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants