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

Creation of InputTextArea component #11710

Merged
merged 130 commits into from Jun 21, 2022

Conversation

Valerian-Perez-Wanadev
Copy link
Contributor

Based on fneitzel prototype (Added prototype of InputTextArea), I improved the component to support the margin, the deletion (backspace/del.), the highlighting, deadkeys and copy/paste/cut shortcuts.

A lot of things remained to do :

  • fix the CR on last line
  • find why the text clip on single line during autostreching
  • resolve todos
  • clean/fix/update the code

My approach was highly naïve, so maybe a cleaner architecture will be necessery.

@sebavan sebavan marked this pull request as draft January 5, 2022 14:39
@fneitzel
Copy link
Contributor

fneitzel commented Jan 5, 2022

Cool that you are finishing it, let me know if you need help.

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

Wow, thank you so much for the. Such a wonderful addition to the GUI!

I have a few comments. The code should be cleaned and formatted, but those are minor issues.

My bigger question is this - I noticed you implemented yourself backspace, delete and so on. This is, eventually, a 3d-controlled <textarea> element. Why not use a textarea as the backend of this component and allow the browser to do the heavy-lifting of text editing?

gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
@RaananW
Copy link
Member

RaananW commented Jan 10, 2022

I also wait for @DarraghBurkeMS's review to test the actual functionality.

@Popov72
Copy link
Contributor

Popov72 commented Jan 10, 2022

Why not use a textarea as the backend of this component and allow the browser to do the heavy-lifting of text editing?

What about Native if using a textarea HTML element under the hood?

@RaananW
Copy link
Member

RaananW commented Jan 10, 2022

What about Native if using a textarea HTML element under the hood?

That's a good point. @bghgary , is there some corresponding element to be used, or does the functionality needs to be (re) implemented?

@darraghjburke
Copy link
Contributor

I'm not Gary, but: I don't think we can depend on any external text area implementation in Babylon Native. One of the critical design choices is that it's "bring your own UI", so we can't rely on any particular one of the many cross-platform UI solutions out there. The goal AFAIK is to implement enough of the canvas API that we can support Babylon GUI, but Babylon Native has no plans to support any DOM features (at that point we'd be making a browser.)

From my POV, having a Babylon GUI text area brings a lot of value, but it's also worth keeping in mind that it will probably be significantly harder to maintain than our existing controls. That being said, I love that you've taken a stab at it, and I'll definitely give it a try later this week :)

@deltakosh
Copy link
Contributor

Totally aligned with @darraghjburke : no external dependencies beside just canvas API

@bghgary
Copy link
Contributor

bghgary commented Jan 10, 2022

What about Native if using a textarea HTML element under the hood?

Maybe @CedricGuillemet can answer this part.

In general, I'm also curious if there are any localization concerns with this change.

@Valerian-Perez-Wanadev
Copy link
Contributor Author

Thanks a lot for all your feedbacks, I'll fix that later in the week!

@CedricGuillemet
Copy link
Contributor

What about Native if using a textarea HTML element under the hood?

Maybe @CedricGuillemet can answer this part.

Hard to tell without testing. At the current state, (basic) text and shape rendering will work. Idk if any unimplemented method is needed here.

@deltakosh
Copy link
Contributor

deltakosh commented Jan 12, 2022

@Valerian-Perez-Wanadev : @fneitzel is interested to contribute to your PR. Do you mind opening your branch for his PR?

Anyway I recommend the 2 of you to chat as your code is based on @fneitzel one

Copy link
Member

@RaananW RaananW 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 great!!

I just have a question about keyCode vs. key (and just a small suggestion that can be ignored).
Is it possible to see a working playground with this in the snapshot playground?

packages/dev/gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
packages/dev/gui/src/2D/controls/inputTextArea.ts Outdated Show resolved Hide resolved
@matthewpflueger
Copy link

Would love to see this merged! This is a perfect fit for our project...

Co-authored-by: Raanan Weber <raananw+github@gmail.com>
@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@Valerian-Perez-Wanadev
Copy link
Contributor Author

Valerian-Perez-Wanadev commented Jun 21, 2022

@RaananW Just one more thing before merging. It's possible to assign @fneitzel as co-author on the changelog please? Originally it was his code and we work sometimes together on this final version.

@RaananW RaananW merged commit e6ebec4 into BabylonJS:master Jun 21, 2022
@RaananW
Copy link
Member

RaananW commented Jun 21, 2022

@RaananW Just one more thing before merging. It's possible to assign @fneitzel as co-author on the changelog please? Originally it was his code and we work sometimes together on this final version.

I will have to edit the changelog afterwards, as it is generated automatically, but i'd be happy to do that. Please remind me after the next version is merged :-)

Also a request regarding this PR - we will need to update the documentation with this new feature. Adding one or two working examples will also be wonderful. Will you be able to do that?

@Valerian-Perez-Wanadev
Copy link
Contributor Author

@RaananW Just one more thing before merging. It's possible to assign @fneitzel as co-author on the changelog please? Originally it was his code and we work sometimes together on this final version.

I will have to edit the changelog afterwards, as it is generated automatically, but i'd be happy to do that. Please remind me after the next version is merged :-)

Also a request regarding this PR - we will need to update the documentation with this new feature. Adding one or two working examples will also be wonderful. Will you be able to do that?

I'll do that 👍

And my local PG has some beautiful examples!

@jonathanandrewsuk
Copy link

@Valerian-Perez-Wanadev I'd be happy to help out with the docs (I've been using the component for a few weeks already)

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