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 issues with escapings with forms #456

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

horcsinbalint
Copy link
Member

@horcsinbalint horcsinbalint commented Mar 3, 2024

There have been multiple issues with characters being escaped incorrectly. Check #28

It's better and easier to use textareas if someone needs to show previous user input since it is much more established how one can escape HTML as simple text on the page in PHP / Laravel compared to escaping inside of an attribute.
x-input.textarea has been also reworked to have a similar interface to x-input.text and also to use slots instead of attributes to show values input by the user (https://laravel.com/docs/10.x/blade#slots).

This PR should fix #28.

@horcsinbalint horcsinbalint force-pushed the text-area branch 4 times, most recently from 82454ee to 0cf17db Compare March 3, 2024 22:43
@horcsinbalint horcsinbalint marked this pull request as ready for review March 3, 2024 22:49
@horcsinbalint horcsinbalint requested a review from a team as a code owner March 3, 2024 22:49
@horcsinbalint
Copy link
Member Author

Fixed conflicts caused by #454

Copy link
Contributor

@kdmnk kdmnk left a comment

Choose a reason for hiding this comment

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

So after this change when should one use the simple text field? Why don't we delete the simple text field and use only the textarea?

@horcsinbalint
Copy link
Member Author

So after this change when should one use the simple text field? Why don't we delete the simple text field and use only the textarea?

As I understand, currently our x-input.text implementation works and used as if it was just a fancy <input> tag. We use it for types other than text (like number). For them, textareas would not be great (they work well with texts but they cannot have a different type).

I was planning on making a simple bugfix, and did not check whether some other components are necessary (like datepicker, timepicker). I'm not a master of tailwind-css, so I just tried to fix the problem that I came across. We should probably create a main x-input.input component, and parameterize it properly in other types of input components (like datepicker, timepicker, etc.). I will check on it.

I just now realized that I just removed the feature that can repopulate the forms in case of validation errors. I will check on that as well.

@kdmnk
Copy link
Contributor

kdmnk commented Mar 10, 2024

Yeah these input fields can be cumbersome to use. The main reason I added those to insert the validation messages automatically as those were easy to miss out (and to avoid writing some trivial tags). I'm open to other solutions here.

When working with input fields we need to keep the followings in mind:

  • validation error message are shown
  • previous value from database is prefilled
  • previous session data is prefilled if exists (after validation error we keep the state)

I'm still sure we don't have all these everywhere yet as there are a lot of special cases.

@kdmnk kdmnk added the on hold label Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix epistola bugs
2 participants