Form: Prepend Submit Button when submitButton is set
#77
Conversation
submitButton is setsubmitButton is set
src/Form.php
Outdated
| return; | ||
| } | ||
|
|
||
| $attr = clone $this->submitButton->getAttributes(); |
There was a problem hiding this comment.
Clone the entire element, since the name must also be the same, or am I mistaken?
There was a problem hiding this comment.
I didn't want to clone the whole Submit Element since it doesn't support Deep Cloning, only shallow. I don't know if this would have side effects or Unknown Behavior since I'm changing the Element's styling.
src/Form.php
Outdated
| } | ||
|
|
||
| $attr = clone $this->submitButton->getAttributes(); | ||
| $attr->setAttribute(Attribute::create('style', 'display: none')); |
There was a problem hiding this comment.
Use this style instead:
border: 0;
height: 0;
margin: 0;
padding: 0;
visibility: hidden;
width: 0;
Else, Safari will just ignore the submit button.
There was a problem hiding this comment.
With these styles, the Element takes up space on top of the Form, resulting in an unwanted Margin. This happens in Chrome and Safari. When using display: none the Element takes up no space. Am I missing something?
There was a problem hiding this comment.
Yes, functionality ;-) The main purpose of such a button is to pick the "correct" submit element when the user presses the Enter/Return key. With display: none behavior depends on the web browser implementation, it might not behave as expected.
Icinga Director does this since v1.0 the following way:
role: none;
tabindex: -1
position: absolute;
left: -100%;There might be better alternatives, but this seems to do it's job quite well.
src/Form.php
Outdated
| public function render(): string | ||
| { | ||
| // Refactor into `ON_ASSEMBLE` once available | ||
| $this->prependSubmitButton(); |
There was a problem hiding this comment.
Calling render more than once also prepends the submit element more than once. This must not happen. Please also add a test.
There was a problem hiding this comment.
While I'd love to see this being implemented for all of our forms, it's IMHO not a good thing to enforce it for every ipl\Html\Form. ipl/html should provide basic building blocks for any kind of HTML, it shouldn't do opinionated magic.
What about providing this method here, giving it a meaningful name (prependedClonedSubmitbutton()?) and not calling it per default? This could then take place in opinionated subclasses, with ipl\web being a good candidate.
There was a problem hiding this comment.
Why would I not want to clone the button by default? The form already supports a primary submit button and additional submit elements. I don't see why this should not be supported by default.
…en if rendering multiple times.
9f50c54 to
35417a6
Compare
src/Form.php
Outdated
| 'style', | ||
| 'border: 0;height: 0;margin: 0;padding: 0;visibility: hidden;width: 0;position: absolute;' |
There was a problem hiding this comment.
Inline style is not an option. We can't use that if we want to support Icinga/icingaweb2#4528 in the future.
Though, the only alternative is to provide the first css asset here. That's been a thing for ipl-web only so far, but I see no other solution. A dependency upon Icinga Web or ipl-web here is also not an option. So ipl-html has to provide it by itself.
There was a problem hiding this comment.
Or do we just give it a class and leave the styling to something else like we do with the decorators?
There was a problem hiding this comment.
leave the styling to something else
If that's done by a library like this, without a dependency, I'd be not pleasantly surprised.
It's either an asset here, or .. an idea I had just recently: Make it a trait, put it in ipl-web. (It's mainly an integration "thing", not a HTML or Form "thing") The CSRF trait is also in ipl-web for that reason.
There was a problem hiding this comment.
like we do with the decorators
they are optional, which makes it different to this.
There was a problem hiding this comment.
an idea I had just recently: Make it a trait, put it in ipl-web. (It's mainly an integration "thing", not a HTML or Form "thing") The CSRF trait is also in ipl-web for that reason.
That's what @Thomas-Gelf would also like 😆 Yeah, let's do it like that!
No description provided.