Add @ to onClick in BsModal::Footer submit button

[SanderKnauff]

As part of the Typescript conversion of bs-modal, I noticed that the @ on @onClick was missing for the submit button.

I have added tests for the submit and close button, and they seem to be fine with or without the @, but to keep in line with Ember's usual style, we should put the @ in there anyway.

[jelhan]

Great catch. My assumption would be that onClick seems to work as well for most use cases as Ember sets the onclick property of the button element.

If that assumptions is correct, the handleClick logic of <BsButton> would be skipped:

ember-bootstrap/ember-bootstrap/addon/components/bs-button.ts

Lines 421 to 457 in 7b66df9

	/**
	* @method click
	* @private
	*/
	@action
	async handleClick(e: MouseEvent) {
	const { bubble, onClick, preventConcurrency } = this.args;
	if (typeof onClick !== 'function') {
	return;
	}
	// Shouldn't we prevent propagation regardless if `@onClick` is a function?
	if (!bubble) {
	e.stopPropagation();
	}
	if (preventConcurrency && this.isPending) {
	return;
	}
	this.state = 'pending';
	try {
	await onClick(this.args.value);
	if (!this.isDestroyed) {
	this.state = 'fulfilled';
	}
	} catch (error) {
	if (!this.isDestroyed) {
	this.state = 'rejected';
	}
	throw error;
	}
	}

This should have the affect that promise state is not handled correctly with the button.

The promise state is used in <BsButton> to control the disabled argument. If my assumption is correct, the button is not disabled while a promise triggered by it is still running. That would be a bug, which is just not covered by our existing tests.

Would be great if you could double check that. Maybe even add a test case for it.

[SanderKnauff]

After testing your assumption seems to be correct. It uses the browser's onclick rather than delegating it to BsButton. I have a hard time getting the test to work as expected however.

For some reason, the isPending getter of BsButton does not update after the _state field changes, which in turn causes the disabled state not to update. Something with Ember's tracking is doing something weird and I can yet put my finger on what it is.

Edit:
The autotracking was not tracking because the disabled attribute of the element was being overwritten by the BsModal::Footer. I'll update with the test

[SanderKnauff]

To get the pending state to work as expected, I had to create a modifier for conditionally setting an attribute on an element. The disabled attribute on the Footer submit button was interfering with the default disabled behavior on BsButton.
In the changed situation, the disabled attribute will only exist on the footer if @submitDisabled is set. In all other cases, BsButton, or the button component provided by the consumer, will handle the disabled state.

[jelhan]

Thanks a lot. Didn't had the time for a detailed review yet. But what I have seen so far looks great. Only one small change to dependency management.

[SanderKnauff]

Thanks a lot. Didn't had the time for a detailed review yet. But what I have seen so far looks great. Only one small change to dependency management.

That's fine! I don't see the comment about the dependency management though. Should something be changed in ember-bootstrap/package.json?

[jelhan]

Thanks a lot. Didn't had the time for a detailed review yet. But what I have seen so far looks great. Only one small change to dependency management.

That's fine! I don't see the comment about the dependency management though. Should something be changed in ember-bootstrap/package.json?

Arghs. Somehow my review comment got lost. In short:

I think ember-modifier should be a peer dependency. It is added as a dependency for every app in Ember CLI blueprints.

Ember Bootstrap introduces a peer dependency on ember-modifier indirectly already today. Ember Bootstrap depends on ember-style-modifier. Ember-style-modifier has a peer dependency on ember-modifier.

Supported range for ember-modifier peer dependency should be the same as ember-style-modifier supports. In that case it wouldn't be a breaking change. At least in my understanding of our informal SemVer commitment for dependency management.

[SanderKnauff]

Ember Bootstrap introduces a peer dependency on ember-modifier indirectly already today. Ember Bootstrap depends on ember-style-modifier. Ember-style-modifier has a peer dependency on ember-modifier.

Supported range for ember-modifier peer dependency should be the same as ember-style-modifier supports. In that case it wouldn't be a breaking change. At least in my understanding of our informal SemVer commitment for dependency management.

Ah, that makes sense. I've check ember-style-modifier, but that does not seem to specifically declare ember-modifier as peer dependency. I've copied version the range of ember-modifier that ember-style-modifier supports now

[jelhan]

I've check ember-style-modifier, but that does not seem to specifically declare ember-modifier as peer dependency.

Good catch! I assumed it does. But it does not. Same for ember-popper-modifier. Both declare it as dependencies only. I guess that should be changed upstream as well at some point.

[jelhan]

Looks good to me. Especially the improved test coverage. Great job!