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

yield submitButton component from <BsForm> #1205

Merged
merged 4 commits into from Aug 31, 2020

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Aug 30, 2020

Adds a submitButton component yielded by <BsForm>. It's a <BsButton> with having @buttonType="submit" and @type="primary". It's disabled if the form is submitting.

This new API helps to reduce the code needed to implement a common submit button. It also improves readability in my opinion as it directly states that the button belongs to the form and will submit it.

{{! new API added by this PR }}
<BsForm as |form|>
  <form.submitButton>Submit</form>
</BsForm>

{{! the same with current API }}
<BsForm as |form|>
  <BsButton @buttonType="submit" @type="primary" @disabled={{form.isSubmitting}}>Submit</BsButton>
</BsForm>

{{! minimal code needed for a submit button with current API }}
<BsForm as |form|>
  <BsButton @buttonType="submit">Submit</BsButton>
</BsForm>

Additionally the submission state of the form is bound to the button state. This is helpful for showing submission state on the submit button.

Using it in isolation would look like this:

<BsForm as |form|>
  <form.submitButton as |button|>
    {{#if button.isPending}}
      <FaIcon @icon="spinner" />
    {{/if}}
    {{#if button.isFulfilled}}
      <FaIcon @icon="check" />
    {{/if}}
    {{#if button.isRejected}}
      <FaIcon @icon="times" />
    {{/if}}
  </form.submitButton>
</BsForm>

This doesn't provide much value over using {{form.isSubmitting}}, {{form.isSubmitted}} and {{form.isRejected}}. But it gets powerful if a user customizes the <BsButton> component to show the state the button is currently. In that case the submission state integration comes out of the box if just doing this:

<BsForm as |form|>
  <form.submitButton />
</BsForm>

Such a customization could be achived by overwriting <BsButton> in the application. Similar to this:

// app/components/bs-button.hbs

<BsButton::Original as |button|>
  {{#if button.isPending}}
    <FaIcon @icon="spinner" />
  {{/if}}
  {{#if button.isFulfilled}}
    <FaIcon @icon="check" />
  {{/if}}
  {{#if button.isRejected}}
    <FaIcon @icon="times" />
  {{/if}}
</BsButton>
// app/components/bs-button/original.js

export { default } from 'ember-bootstrap/components/bs-button';

This is one of the two outstanding features from #638.

@jelhan
Copy link
Contributor Author

jelhan commented Aug 30, 2020

We could implement submit for correctly positioning the button in horizontal forms if using Bootstrap 3 as discussed in #92. If we want to we should do it now before shipping the feature in a stable release as it would be a breaking change otherwise. But I'm not sure if it makes sense to still invest time in Bootstrap 3 to be honest.

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Thanks @jelhan, this looks great!

We could implement submit for correctly positioning the button in horizontal forms if using Bootstrap 3 as discussed in #92.

I would tend to think that it's not really the concern of the button to determine its position. Rather something like a special yielded layout component or so, that uses the plain button. But I also think we don't need to add things for BS3 specifically anymore :)

I just wanted to check what breaks CI for beta and canary, so we can get all PRs green...

{{#if form.isSubmitting}} {{fa-icon "spinner"}} {{/if}}
</BsButton>
{{#if form.isSubmitting}}
<FaIcon @icon="spinner" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use a plain BS spinner example here, like this https://getbootstrap.com/docs/4.5/components/spinners/?

Juts in case people copy and paste this and wonder why it's not working ootb!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 41ffa5b. Also noticed that it might be good practice to not only show an icon but also have a text - at least for screen readers only (accessibility).

* @private
*/
@defaultValue
submitButtonComponent = 'bs-button';
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me that we should get rid of that pattern sooner than later to make component tree-shakable. It's ok for now, as we have that all over the place, but I just created this issue to not forget this: #1206

@jelhan jelhan force-pushed the yield-submit-button-component-in-form branch from 1d6afc5 to b5ede20 Compare August 31, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants