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

test: Handle React form inputs #9394

Closed
wants to merge 2 commits into from

Conversation

mvollmer
Copy link
Member

No description provided.

Setting the value plus emitting the "change" event is not enough.
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Setting the checked property and emitting the "change" signal does not
work with React.
@@ -231,6 +231,11 @@ def key_press(self, keys):
else:
self.cdp.invoke("Input.dispatchKeyEvent", type="char", text=k)

def set_input_text(self, selector, val):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would explicitly mention "type" in the name of this function

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'd say "typing" is an implementation detail. If we later find a better way that doesn't involve key-press events, I think we should use it.

@@ -280,7 +280,7 @@ export const PassInput = (tag, title, options) => {
initial_value: "",

render: (val, change) =>
<input data-field={tag}
<input data-field={tag} data-field-type="text-input"
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in other PRs, the data-* attributes are not preferred in React. They exist just for compatibility with (older) libraries.
Wouldn't it be better to determine the type form css class? If not in all cases recently, we can add a new one, without it's definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

We might use a class for data-field-type, but it's more difficult for data-tag. But I don't see why we should stop using data-* attributes. They work, React doesn't warn about them, and there isn't a equivalent alternative, right?

@martinpitt
Copy link
Member

Let's merge this now, as there are quite a lot of other React PRs and these tend to age quickly. It doesn't make the code any worse, and we can iterate on it again.

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

3 participants