refactor(alert): remove intermediate 'div' element #2552
Conversation
src/alert/alert.ts
Outdated
| ngOnChanges(changes: SimpleChanges) { | ||
| const {type} = changes; | ||
| if (type && !type.firstChange) { | ||
| this._renderer.removeClass(this._element.nativeElement, `alert-${type.previousValue}`); |
There was a problem hiding this comment.
Why do we do this through low-level DOM manipulation? Couldn't we move the existing bindings to the host?
Or is binding overriding classes set by a user on a host?
There was a problem hiding this comment.
how would you do this?
[class.alert-warning]: 'type === warning',
[class.alert-success]: 'type === success',
etc?
we also currently support alert-customclass as seen in demos
There was a problem hiding this comment.
[class]="'alert alert-' + type + (dismissible ? ' alert-dismissible' : '')" ?
Exactly as we used to have it, just moved to the host.
I just don't remember if Angular is merging classes on the host or not...
There was a problem hiding this comment.
Double-checked, and it overrides user-set classes. So I see three options:
- Leave as it is done currently with
Renderer2 - hardcode all supported classes and remove the demo with
alert-customclass(breaking change?):
host: {
'role': 'alert',
'class': 'alert d-block',
'[class.alert-dismissible]': 'dismissible',
'[class.alert-success]': `type === 'success'`,
'[class.alert-warning]': `type === 'warning'`,
'[class.alert-info]': `type === 'info'`,
'[class.alert-danger]': `type === 'danger'`,
'[class.alert-primary]': `type === 'primary'`,
'[class.alert-secondary]': `type === 'secondary'`,
'[class.alert-light]': `type === 'light'`,
'[class.alert-dark]': `type === 'dark'`
},
- do nothing and close this :)
src/alert/alert.ts
Outdated
| this.dismissible = config.dismissible; | ||
| this.type = config.type; | ||
| } | ||
|
|
||
| closeHandler() { this.close.emit(null); } | ||
|
|
||
| ngOnChanges(changes: SimpleChanges) { | ||
| const {type} = changes; |
There was a problem hiding this comment.
While it works I find the type name confusing / not very descriptive here. A much better name would be typeChange. I would prefer to be explicit here.
src/alert/alert.ts
Outdated
| } | ||
| } | ||
|
|
||
| ngOnInit() { this._renderer.addClass(this._element.nativeElement, `alert-${this.type}`); } |
There was a problem hiding this comment.
Couldn't we move this logic to ngOnChanges and set the class on firstChange? This way the whole logic would be centralised in one place
There was a problem hiding this comment.
In this case:
<ngb-alert>Cool!</ngb-alert>
onChanges won't be called at all, but initial value comes from the NgbAlertConfig
|
@maxokorokov I left few nits but generally speaking I think that we should merge it. It is not an ideal solution but this is the best we've got given current Angular... |
maxokorokov
force-pushed
the
alert/refactor/simplify-markup
branch
from
2b4fe7f
to
9c1c915
Compare
maxokorokov
force-pushed
the
alert/refactor/simplify-markup
branch
from
9c1c915
to
f223de0
Compare
BREAKING CHANGE: markup generated by `<ngb-alert>` was simplified, there is no more intermediate `<div>` element
Before:
```
<ngb-alert>
<div role="alert" class="alert alert-warning">
Hello there
</div>
</ngb-alert>
```
After:
```
<ngb-alert role="alert" class="alert alert-warning">
Hello there
</ngb-alert>
```
Fixes ng-bootstrap#2544
maxokorokov
force-pushed
the
alert/refactor/simplify-markup
branch
from
f223de0
to
217d94c
Compare
|
@pkozlowski-opensource , fixed comments and:
|
Fixes #2544