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

feat(button): submit from outside of form #25913

Merged
merged 15 commits into from Sep 30, 2022

Conversation

postnerd
Copy link
Contributor

@postnerd postnerd commented Sep 9, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • 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.
  • Build (npm run build) was run locally and any changes were pushed
  • 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
  • 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: #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
  • 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 postnerd requested a review from a team as a code owner September 9, 2022 23:37
@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package labels Sep 9, 2022
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Small suggestion on the documentation (will require a new npm run build for the generated outputs).

We will also need Playwright tests for this feature.

For example, in core/src/components/button/test/form-reference create a new button.e2e.ts file.

You will need to test 3 conditions:

  1. Form with an ID
  2. Form as a reference
  3. No form (querying the closest form).

For these, setting the HTML mark-up per test is probably easiest.

I did not run these tests locally, but something like this:

test.describe('button: form', () => {

  test('should submit the form by id', async({ page }) => {
    await page.setContent(`
      <form id="myForm"></form>
      <ion-button form="myForm" type="submit">Submit</ion-button>
    `);

    const submitEvent = await page.spyOnEvent('submit');
    
    await page.click('ion-button');

    expect(submitEvent).toHaveReceivedEvent();
  });

  test('should submit the form by reference', async({ page }) => {
    await page.setContent(`
      <form></form>
      <ion-button type="submit">Submit</ion-button>
      <script>
        const form = document.querySelector('form');
        const button = document.querySelector('ion-button');

        button.form = form;
      </script>
    `);

    const submitEvent = await page.spyOnEvent('submit');
    
    await page.click('ion-button');

    expect(submitEvent).toHaveReceivedEvent();
  });

  test('should submit the closest form', async({ page }) => {
    await page.setContent(`
    <form>
      <ion-button type="submit">Submit</ion-button>
    </form>
  `);

  const submitEvent = await page.spyOnEvent('submit');
  
  await page.click('ion-button');

  expect(submitEvent).toHaveReceivedEvent();
  });
});

Let me know if you have questions.

core/src/components/button/button.tsx Outdated Show resolved Hide resolved
@postnerd
Copy link
Contributor Author

@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.

@liamdebeasi
Copy link
Member

@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.

Copy link
Contributor Author

@postnerd postnerd left a comment

Choose a reason for hiding this comment

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

Added the required tests.

Please let me know if there is anything else to do.

(Messed up this one commit when adding the tests with a wrong commit message.) Since this is such a small PR I could just create another branch and PR with just one commit if you want me to.)

@sean-perkins
Copy link
Contributor

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.

@postnerd
Copy link
Contributor Author

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

@sean-perkins
Copy link
Contributor

sean-perkins commented Sep 19, 2022

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

@postnerd
Copy link
Contributor Author

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

@sean-perkins
Copy link
Contributor

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
Copy link
Contributor

@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 sean-perkins requested a review from a team as a code owner September 26, 2022 18:51
@sean-perkins sean-perkins changed the base branch from main to feature-6.3 September 26, 2022 19:00
@sean-perkins sean-perkins requested a review from a team September 26, 2022 19:43
Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

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

LGTM once the comment is addressed 👍

core/src/components/button/button.tsx Outdated Show resolved Hide resolved
);
} else {
printIonWarning(
`The provided "form" element could not be found. Verify that the form is rendered in the DOM.`,
Copy link
Member

Choose a reason for hiding this comment

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

This warning is a bit confusing because it implies that the form element is not in the DOM at all. However, it could be the case that the element is in the DOM but is not an HTMLFormElement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the warning message. Let me know if it is more clear or requires additional adjustment.

core/src/components/button/button.tsx Show resolved Hide resolved
core/src/components/button/test/form-reference/index.html Outdated Show resolved Hide resolved
@sean-perkins
Copy link
Contributor

Great work here @postnerd 👍 thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants