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

fix: Improve Usability of Adding Options via Return Key for Multi-Select Field #7450

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

nganphan123
Copy link
Contributor

@nganphan123 nganphan123 commented Oct 5, 2024

Fixes #6602

This is the approach that I followed based on these comments #6602 (comment), #6602 (comment)

  • Create forward ref in <TextInput> component
  • Create ref to select text in parent component <SettingsDataModelFieldSelectFormOptionRow> and pass it to <TextInput>

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses issue #6602 by improving the usability of adding options via the Return key for multi-select fields. The changes focus on enhancing the text selection behavior when a new option is added.

  • Implemented forwardRef in TextInput component to allow parent components to access the input element directly
  • Added hasFocused state and handleInputRef callback in SettingsDataModelFieldSelectFormOptionRow to manage focus and text selection
  • Modified TextInput to use useCombinedRefs hook, combining the forwarded ref with the internal inputRef
  • Updated SettingsDataModelFieldSelectFormOptionRow to pass a ref prop to TextInput, enabling automatic text selection for new options

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -88,6 +89,17 @@ export const SettingsDataModelFieldSelectFormOptionRow = ({
onInputEnter?.();
};

const handleInputRef = useCallback(
(node: HTMLInputElement | null) => {
if (Boolean(node) && !hasFocused && Boolean(focused)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider using node && !hasFocused && focused instead of Boolean(node) && !hasFocused && Boolean(focused) for consistency and readability

@nganphan123
Copy link
Contributor Author

Hi @FelixMalfait , thank you for reviewing my previous PR. Please let me know if the current way is the right approach. Otherwise, I'm happy to take another direction. Thank you!

@charlesBochet
Copy link
Member

Hi @nganphan123, the way you have chosen seems quite hacky to me:

  • putting a callback with logic within a ref (on the project refs are limited to dom manipulation only)
  • adding a useEffect in the TextInput when we can trigger behaviors synchronously and more precisely (here enter press)

I'm investigating a bit to give you more precise hints

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Oct 7, 2024

Hi,

The difficulty here was to work at the right abstraction layer.

We wanted the selection to happen on new select option row, so the component SettingsDataModelFieldSelectFormOptionRow was the most indicated to encode this intent.

The intent was "the new option row should be selected and focused on mount" so I divided this into other intents :

  • The input can be focused on mount => new props autoFocusOnMount in TextInput (which is purely generic)
  • The input can be selected on mount => new props autoSelectOnMount in TextInput (which is also purely generic)
  • The SettingsDataModelFieldSelectFormOptionRow should know if its a new row => new props isNewRow

Then in the option row .map, we just tell each row if it's a new row or not.

@lucasbordeau lucasbordeau merged commit 2bc7974 into twentyhq:main Oct 7, 2024
11 checks passed
Copy link

oss-gg bot commented Oct 7, 2024

Awarding nganphan123: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/nganphan123

Copy link

github-actions bot commented Oct 7, 2024

Thanks @nganphan123 for your contribution!
This marks your 3rd PR on the repo. You're top 9% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
…ect Field (twentyhq#7450)

Fixes twentyhq#6602

This is the approach that I followed based on these comments
twentyhq#6602 (comment),
twentyhq#6602 (comment)
- Create forward ref in `<TextInput>` component
- Create ref to select text in parent component
`<SettingsDataModelFieldSelectFormOptionRow>` and pass it to
`<TextInput>`

---------

Co-authored-by: Lucas Bordeau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Usability of Adding Options via Return Key for Multi-Select Field
3 participants