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: expose onFocus and onBlur functions on TextAreaEntry #191

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

ogmzh
Copy link
Contributor

@ogmzh ogmzh commented Oct 21, 2022

The inner component TextArea spreads the props.onBlur and props.onFocus functions to the native textarea component:

<textarea
        ....
        onFocus={ props.onFocus }
        onBlur={ props.onBlur }
        ... />

But the outer component TextAreaEntry never actually exposes the same props to the consumer.

@CLAassistant
Copy link

CLAassistant commented Oct 21, 2022

CLA assistant check
All committers have signed the CLA.

@marstamm
Copy link
Contributor

Hi @zhGio , thank you for opening this PR. It seems that this is missing from all components except simple.

Do you think it makes sense to add them to

<Textfield
debounce={ debounce }
disabled={ disabled }
id={ id }
key={ element }
label={ label }
onInput={ onInput }
value={ value } />
{ error && <div class="bio-properties-panel-error">{ error }</div> }
and
<FeelTextfield
debounce={ debounce }
disabled={ disabled }
feel={ feel }
id={ id }
key={ element }
label={ label }
onInput={ onInput }
example={ props.example }
show={ show }
value={ value }
variables={ props.variables }
OptionalComponent={ props.OptionalComponent } />
as well?

@ogmzh
Copy link
Contributor Author

ogmzh commented Oct 28, 2022

Hi, @marstamm

Yeah, I think it would make sense to expose those events to consumers on all components that render native components capable of focus/blur. I just ran into the issue of not having them on the TextArea while trying to build a feature.
Do you want me to add them to the rest of the components in this PR, open up a new PR, or is that something you guys would do? (if you agree those events should be exposed in the first case :))

@marstamm
Copy link
Contributor

Hi,
Yes, I think these should be exposed. I see no reason why we would hide them when they can be useful :)
It would be a great help if you could add them to this PR, so we have consistent API across all fields. Thanks again for your contribution!

@ogmzh
Copy link
Contributor Author

ogmzh commented Oct 28, 2022

Hi @marstamm, updated this PR with a lot more changes so if you could please review :)

I've manually tested some of the examples and ran all the tests, seems ok to me.

P.S. the force-push was me changing the author of the updated commit as i accidentally committed from my company account 😅

@ogmzh ogmzh force-pushed the patch-textarea-focus-blur branch from 13f9a5f to 35692da Compare October 28, 2022 15:54
@marstamm
Copy link
Contributor

Thank you for the follow-up. CI is happy as well, so let's merge it 🚀

@marstamm marstamm merged commit d9f630b into bpmn-io:main Oct 31, 2022
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.

3 participants