ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

feat(button): submit from outside of form

Open postnerd opened this issue 3 years ago • 7 comments
trafficstars

Pull request checklist

Please check if your PR fulfills the following requirements:

  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • [x] Build (npm run build) was run locally and any changes were pushed
  • [x] Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [ ] Other (please describe):

What is the current behavior?

Buttons with type "submit" will only work from inside forms.

Issue URL: https://github.com/ionic-team/ionic-framework/issues/21194

What is the new behavior?

Now you can add a "form" prop to every that will be used, if the button is placed outside of a form. This can be helpful, if the button is added to a header/footer toolbar but should be responsible for a specific form.

Does this introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

Not sure, if the docs have to be updated. If so, I'm happy to do so. Maybe you could guide me to the right place.

postnerd avatar Sep 09 '22 23:09 postnerd

@sean-perkins Thanks for the support. But I can't get the tests to run.

I've tried all possible commands for testing npm run test.screenshot or npm run test or npm run run test.e2e but only the plain e2e.ts files will be executed not the button.e2e.ts files.

npm test src/components/button/test/basic/button.e2e.ts will run into the following error message Pattern: src/components/button/test/basic/button.e2e.ts - 0 matches.

postnerd avatar Sep 14 '22 08:09 postnerd

@postnerd We are in the middle of a transition between two E2E testing systems. The npm run test.e2e command is for the old system. You should do npx playwright test src/components/button/test/basic to run the new Playwright tests.

liamdebeasi avatar Sep 14 '22 13:09 liamdebeasi

The commit message is ok here. We will end up squashing the commits anyways, so only the topmost description (the PR title) will matter.

I approved CI to run to verify the tests. I don't see any other items requiring changes on my end.

If CI passes, I'll add the team and we can discuss the release target for this feature.

sean-perkins avatar Sep 14 '22 19:09 sean-perkins

@sean-perkins Is there anything to do from my side because of the failed tests?

postnerd avatar Sep 16 '22 14:09 postnerd

@postnerd merge the latest change from main into this branch, run npm run build (from the core folder) and commit any changes (if any).

sean-perkins avatar Sep 19 '22 15:09 sean-perkins

Merged again (after @liamdebeasi already merged) because the branch was again one commit behind.

npm run build didn't bring anything up related to this pull request.

Only one change to angular/src/directives/proxies-list.ts that I've ignored before, because it looks like an unrelated style fix (a strange one, adding a line at the beginning of the file ...).

Screenshot 2022-09-19 at 21 49 31 Screenshot 2022-09-19 at 21 49 39

postnerd avatar Sep 19 '22 19:09 postnerd

Yeah, you can ignore that diff. Running npm run prettier from the /angular folder will overwrite that change and reformat the file with the current state.

I re-triggered CI to run.

sean-perkins avatar Sep 19 '22 20:09 sean-perkins

@postnerd update on our end: The team has drafted and approved a design document around this feature. The only item that is outstanding as a result, is we want to introduce a developer warning when the form cannot be found, when using the form syntax.

I can snag this scope & can commit it directly to your fork.

We will target this feature for 6.3 after I make that change & then the team can do a final review.

sean-perkins avatar Sep 26 '22 16:09 sean-perkins

Great work here @postnerd 👍 thanks for the contribution!

sean-perkins avatar Sep 30 '22 18:09 sean-perkins