-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Convert Form::textarea to blade component #16075
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
Convert Form::textarea to blade component #16075
Conversation
PR Summary
These changes promote a uniform implementation of the text area component, thus making the platform more synchronized and manageable. |
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]> # Conflicts: # public/css/dist/all.css # public/css/dist/bootstrap-table.css # public/js/dist/bootstrap-table.js # public/mix-manifest.json
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]> # Conflicts: # config/version.php
snipe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have a conflict here, and this is targeted to master
|
I think I fixed the conflict, but the branch switch is a bit messy - possible for you to rebase or merge down? |
|
I'll take a look. |
|
I am, of course, in the middle of adding textareas to a bunch of forms right now lolsob. I'll put them in as in my PR and then update them to this format once it's targeted to develop and merged. |
|
Looks like Sqlite tests are failing |
|
The sqlite tests are due to a mismatched timestamp. Another run should get them to green. Now that it is caught up to master I'm not sure how to do the branch change cleanly... |
|
I'm guessing you must have branched off of master to start with, based on the changes (and the version.php). Please also make sure you are using translation strings for placeholder text. |
| @@ -0,0 +1,11 @@ | |||
| @props([ | |||
| 'value' => '', | |||
| 'cols' => 50, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed this, but we don't actually use cols, since we let bootstrap handle the width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Form::textarea call actually added those in there so the resulting html included it. Should I remove it as an prop in the component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably - it won't matter, since the CSS will override it, but we don't need it. Not urgent tho. The behavior will be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did it so I didn't forget about it 😅
Those changes came along with the "catch up" merge from what I can tell. I think if the order was flipped and the target branch was changed to
My intention was to do the replacements as simply as possible and avoid extracting new translation entries for the untranslated strings I came across. I can open a follow up PR extracting those though. |
I get it, but we always end up missing stuff. Hang on for adding those, since I'm creating a new array for placeholders in general - you can add to that. |
This PR introduces the
<x-input.textarea>blade component and uses it to replaceForm::textareausage in our blade templates.Pages Affected
Modals Affected
Extracted via copy/paste from #15983.