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

Issue/dialog autofocus #137

Closed
wants to merge 2 commits into from
Closed

Issue/dialog autofocus #137

wants to merge 2 commits into from

Conversation

Xaohs
Copy link
Contributor

@Xaohs Xaohs commented Oct 1, 2024

No description provided.

@Xaohs Xaohs requested a review from svenvandescheur October 1, 2024 14:46
@Xaohs Xaohs force-pushed the issue/dialog-autofocus branch from 7e2eb30 to c601399 Compare October 1, 2024 14:47
undefined,
{ allowClose: false, ...modalProps },
);
};

return fn;
};

const PromptForm = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason why an additional component is introduced over using the effect directly on the hook (might work as well I think?)

I like to keep changes as minimal as possible especially considering that a new similar hook useFormDialog may be introduced (see: OAB).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a hook cannot be used in the function of that component like that, as it defies the rules of react hooks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I think you're correct, were acting on the returned function instead of the hook. I sill think this could be a nice moment to merge this with useFormDialog though so lets discuss further.

@Xaohs Xaohs requested a review from svenvandescheur October 3, 2024 08:36
@Xaohs
Copy link
Contributor Author

Xaohs commented Oct 4, 2024

Closed in favor of #140

@Xaohs Xaohs closed this Oct 4, 2024
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.

2 participants