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

CompatForm: Insert duplicate submit button when more than one is present #104

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

TAINCER
Copy link
Contributor

@TAINCER TAINCER commented Nov 24, 2022

Creates a new SubmitElement from the primary submit button of the form with cloned Attributes from the given FormSubmitElement. The new submit button is always at the top of the form.
The element has a fixed primary-submit-btn-duplicate class, which hides to button, but not the functionality when submitting the form.

@TAINCER TAINCER self-assigned this Nov 24, 2022
@cla-bot cla-bot bot added the cla/signed label Nov 24, 2022
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from e51b859 to 8e44197 Compare November 24, 2022 09:58
@TAINCER TAINCER marked this pull request as ready for review November 24, 2022 10:16
@TAINCER TAINCER requested review from lippserd and removed request for lippserd November 24, 2022 10:17
@nilmerg
Copy link
Member

nilmerg commented Nov 24, 2022

What about overriding setSubmitButton() with this trait? It's used with child classes of ipl\Html\Form after all so it shouldn't pose a problem in the inheritance tree.

I'd like to just use the trait and be done with it. Calling a special method, in this case, is too tedious for me 😉

@TAINCER
Copy link
Contributor Author

TAINCER commented Nov 25, 2022

What about overriding setSubmitButton() with this trait? It's used with child classes of ipl\Html\Form after all so it shouldn't pose a problem in the inheritance tree.

I'd like to just use the trait and be done with it. Calling a special method, in this case, is too tedious for me 😉

if I would use the Trait in setSubmitButton(), the Prefix Button could be rendered multiple times. The commit for CompatForm was still missing, in there I've overwritten renderContent(), where I used the createSubmitButton() method.

@nilmerg
Copy link
Member

nilmerg commented Nov 25, 2022

I see. I forgot that this is only relevant if there are multiple submit buttons and then the set submit button isn't reliable. (even if available)

src/Compat/CompatForm.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch 3 times, most recently from 50044d5 to bb88818 Compare November 25, 2022 13:58
@TAINCER TAINCER requested review from nilmerg and lippserd November 25, 2022 13:59
tests/TestCase.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
src/Common/PrefixSubmitButton.php Outdated Show resolved Hide resolved
src/Common/PrefixSubmitButton.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Show resolved Hide resolved
@nilmerg nilmerg added this to the v0.7.0 milestone Nov 25, 2022
@nilmerg nilmerg added the enhancement New feature or request label Nov 25, 2022
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch 2 times, most recently from c5a5b4f to e540ffb Compare November 28, 2022 09:55
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Please also add a another test case to CompatFormTest which verifies that an explicit call to setSubmitButton() duplicates said submit button, and not the first registered one.

src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
@TAINCER TAINCER requested a review from nilmerg December 1, 2022 13:39
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Please squash your commits to the following:

  • One for the trait + less file
  • One for the composer.json
  • One for the CompatFormTest
  • One for the DuplicateSubmitButttonTest + TestCase

tests/Compat/CompatFormTest.php Show resolved Hide resolved
tests/common/DuplicateSubmitButtonTest.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from c73fa30 to 7cfc922 Compare January 17, 2023 15:19
@TAINCER TAINCER requested a review from nilmerg January 17, 2023 15:19
tests/TestCase.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Show resolved Hide resolved
src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from 7cfc922 to 7dc5d98 Compare January 20, 2023 11:33
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
tests/Common/DuplicateSubmitButtonTest.php Outdated Show resolved Hide resolved
asset/css/prefix-button.less Outdated Show resolved Hide resolved
src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
src/Common/DuplicateSubmitButton.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from 969655b to 43a6800 Compare January 31, 2023 08:50
@TAINCER TAINCER requested a review from lippserd January 31, 2023 08:51
src/Compat/CompatForm.php Show resolved Hide resolved
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
asset/css/primary-submit-btn-duplicate.less Show resolved Hide resolved
@TAINCER TAINCER requested a review from lippserd January 31, 2023 11:53
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

PR title and description have to be adjusted as well.

@TAINCER TAINCER changed the title Add PrefixSubmitButton Trait CompatForm: Insert duplicate submit button when more than one is present. Jan 31, 2023
@TAINCER TAINCER changed the title CompatForm: Insert duplicate submit button when more than one is present. CompatForm: Insert duplicate submit button when more than one is present Jan 31, 2023
@TAINCER TAINCER requested a review from lippserd February 2, 2023 13:24
nilmerg
nilmerg previously approved these changes Feb 8, 2023
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from 0ad95ab to 857b6bf Compare February 14, 2023 10:19
@TAINCER TAINCER requested a review from nilmerg February 14, 2023 10:19
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from e6393f3 to 7b624ba Compare February 17, 2023 15:41
@TAINCER TAINCER requested a review from nilmerg February 17, 2023 15:41
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
src/Compat/CompatForm.php Outdated Show resolved Hide resolved
tests/Compat/CompatFormTest.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from 7b624ba to c508a5d Compare February 20, 2023 09:38
@TAINCER TAINCER requested a review from lippserd February 20, 2023 09:38
@TAINCER TAINCER force-pushed the PrefixSubmitButtonTrait branch from c508a5d to 811318c Compare February 20, 2023 10:06
@nilmerg nilmerg merged commit e0b4e25 into master Feb 23, 2023
@nilmerg nilmerg deleted the PrefixSubmitButtonTrait branch February 23, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants