Skip to content

Commit

Permalink
Refactor bs-alert to remove @localcopy and avoid mutation after consu…
Browse files Browse the repository at this point in the history
…mption error in Ember canary (ember-bootstrap#2027)

* Refactor bs-alert to remove @localcopy

The `@localCopy` decorate caused issues on Ember canary builds.
By refactoring this component to use pure Ember reactivity we can get
rid of the decorator.

* Allow re-enabling of alert using property after hiding it by clicking
  • Loading branch information
SanderKnauff authored and Gaurav0 committed Nov 8, 2024
1 parent a23f927 commit fe95c91
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 17 deletions.
1 change: 1 addition & 0 deletions addon/components/bs-alert.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
class="{{unless this.hidden "alert"}} {{if this.fade "fade"}} {{if this.dismissible "alert-dismissible"}} {{bs-type-class "alert" @type}} {{if this.showAlert (if (macroCondition (macroGetOwnConfig "isNotBS3")) "show" "in")}}"
...attributes
{{did-update this.showOrHide this._visible}}
{{did-update this.updateVisibility @visible}}
>
{{#unless this.hidden}}
{{#if this.dismissible}}
Expand Down
33 changes: 16 additions & 17 deletions addon/components/bs-alert.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { later } from '@ember/runloop';
import usesTransition from 'ember-bootstrap/utils/decorators/uses-transition';
import deprecateSubclassing from 'ember-bootstrap/utils/deprecate-subclassing';
import arg from 'ember-bootstrap/utils/decorators/arg';
import { localCopy } from 'tracked-toolbox';

/**
Implements [Bootstrap alerts](http://getbootstrap.com/components/#alerts)
Expand Down Expand Up @@ -74,12 +73,6 @@ export default class Alert extends Component {
@tracked
hidden = !this.visible;

/**
* This is an unfortunate duplication of the previous property, but this is untracked to avoid causing the dreaded "same computation" assertion in GlimmerVM when reading a tracked property before setting it.
* @private
*/
_hidden = !this.visible;

/**
* This property controls if the alert should be visible. If false it might still be in the DOM until the fade animation
* has completed.
Expand All @@ -92,15 +85,16 @@ export default class Alert extends Component {
* @default true
* @public
*/
@arg
visible = true;
get visible() {
return this._visible ?? true;
}

/**
* @property _visible
* @private
*/
@localCopy('visible')
_visible;
@tracked
_visible = this.args.visible;

/**
* Set to false to disable the fade out animation when hiding the alert.
Expand All @@ -114,7 +108,7 @@ export default class Alert extends Component {
fade = true;

get showAlert() {
return this._visible && this.args.fade !== false;
return this.visible && this.args.fade !== false;
}

/**
Expand Down Expand Up @@ -180,7 +174,7 @@ export default class Alert extends Component {
* @private
*/
show() {
this.hidden = this._hidden = false;
this.hidden = false;
}

/**
Expand All @@ -191,7 +185,7 @@ export default class Alert extends Component {
* @private
*/
hide() {
if (this._hidden) {
if (this.hidden) {
return;
}

Expand All @@ -200,24 +194,29 @@ export default class Alert extends Component {
this,
function () {
if (!this.isDestroyed) {
this.hidden = this._hidden = true;
this.hidden = true;
this.args.onDismissed?.();
}
},
this.fadeDuration
);
} else {
this.hidden = this._hidden = true;
this.hidden = true;
this.args.onDismissed?.();
}
}

@action
showOrHide() {
if (this._visible) {
if (this.visible) {
this.show();
} else {
this.hide();
}
}

@action
updateVisibility() {
this._visible = this.args.visible;
}
}
28 changes: 28 additions & 0 deletions tests/integration/components/bs-alert-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,34 @@ module('Integration | Component | bs-alert', function (hooks) {
assert.dom('.alert').hasClass('alert-success', 'alert has type class');
});

test('alert can be made visible when setting visible=true after manually dismissing it', async function (assert) {
this.set('visible', true);

this.set('handleOnDismissed', () => {
this.set('visible', false);
});

await render(hbs`<BsAlert
@type='success'
@fade={{false}}
@visible={{this.visible}}
@onDismissed={{this.handleOnDismissed}}
>
Test
</BsAlert>`);

assert.dom('.alert').exists('alert has alert class');
assert.dom('button.close').exists({ count: 1 }, 'alert has close button');

await click('button.close');
assert.dom('.alert').doesNotExist('alert has no alert class');
assert.dom('*').hasText('', 'alert has no content');

this.set('visible', true);

assert.dom('.alert').exists('alert has alert class');
});

test('dismissing alert does not change public visible property (DDAU)', async function (assert) {
this.set('visible', true);
await render(hbs`<BsAlert @type="success" @visible={{this.visible}} @fade={{false}}>Test</BsAlert>`);
Expand Down

0 comments on commit fe95c91

Please sign in to comment.