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

Autosize Daily Nerd Textarea #99

Merged
merged 2 commits into from
May 21, 2024
Merged

Autosize Daily Nerd Textarea #99

merged 2 commits into from
May 21, 2024

Conversation

danieldiekmeier
Copy link
Contributor

  • I made sure you can only resize the textarea vertically
  • I added a sprinkle of JavaScript so that textareas get auto resizing
Screen.Recording.2024-05-16.at.18.50.37.mov

@danieldiekmeier danieldiekmeier self-assigned this May 16, 2024
@danieldiekmeier danieldiekmeier added the frontend Requires a frontend developer. label May 16, 2024
@leoggonzalez
Copy link
Contributor

You should consider a pure css solution
https://github.com/KrasserStoff/core/blob/master/connect/src/components/Form/TextArea/TextArea.css

Also is this resolving this issue?
#93

@danieldiekmeier
Copy link
Contributor Author

@leoggonzalez Doesn't this still need JavaScript to fill the data-replicated-value?

Haha, I didn't know #93 exists, sorry. :D I'm only interested in the auto height for now.

@leoggonzalez
Copy link
Contributor

@danieldiekmeier you still need js yes but at least you won't be programatically setting inline styles, which are never desirable.

 this.#el.style.height = this.#el.scrollHeight + 'px';

Also good to know that this will be fixed with one line of css in the future:
https://youtu.be/_-6LgEjEyzE?t=2166

@danieldiekmeier
Copy link
Contributor Author

@leoggonzalez I disagree, I think inline styles do have their place, and that this is a reasonable use case for them.

In the solution I proposed, we only need to attach the Stimulus Controller directly to the textarea, so the impact to HTML and CSS is minimal.

I started implementing the CSS version you linked, but I noticed that it needs additional wrappers which are not currently part of the FormBuilder we're using. So we'd add code to the FormBuiler, to the HTML, to the CSS and to the JS. Feels too shotgun surgery for me.

If you feel strongly that this can't be merged, then I think I'll just close the PR.

@leoggonzalez
Copy link
Contributor

leoggonzalez commented May 21, 2024

@danieldiekmeier sorry i did not know about the hassle of this particular stack. Seemed quite straightforward in our react implementation. I'll just approve this.

Hopefully in the future @JensRavens can be convinced to migrate to react (next?) for the frontend part in this project.

@danieldiekmeier
Copy link
Contributor Author

@leoggonzalez Yessss, in React, this would have been very easy. :D That's what I thought when I looked at the KS code, but when I started copying it over, it quickly became worse and worse. Thanks for the approval!

@JensRavens JensRavens merged commit 8154ba9 into main May 21, 2024
1 check passed
@JensRavens JensRavens deleted the autosize-textarea branch May 21, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Requires a frontend developer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants