Refactor BsAlert to Glimmer component #1224
Conversation
There was a problem hiding this comment.
Looks good to me in general. The requested change is only about the fade property, which doesn't seem to be used.
I noticed (again) some architectural decision, which doesn't seem to make much sense anymore to me. They don't need to be addressed while doing the refactoring. But I wanted to get the discussion rolling as we normally don't dig that deep into the code base.
It boils down to having three different states (this.args.visible, this._visible and this.hidden), which control the visibility of the alert (unless I missed something). That seems to be wrong to me.
addon/components/bs-alert.js
Outdated
| @@ -55,7 +51,7 @@ export default class Alert extends Component { | |||
| * @readonly | |||
| * @private | |||
| */ | |||
| @defaultValue | |||
| @tracked | |||
| hidden = !this._visible; | |||
There was a problem hiding this comment.
This default value seems wrong to me. _visible is an alias for this.visible when creating an instance. So it should be either this.visible or this.args.visible. Or did I missed something?
Are you sure about the timing? Is this.args populated early enough?
To be honest I'm also confused about how this.args.visible, this._visible and this.hidden play together. Three different states coming together to decide if the alert is shown seems to be overcomplex. 🤯
Wondering if we can simplify this as it raises so many questions.
There was a problem hiding this comment.
This default value seems wrong to me. _visible is an alias for this.visible when creating an instance. So it should be either this.visible or this.args.visible. Or did I missed something?
this.args.visible would be wrong, as that would not take the default of true into account. And this.visible === this._visible and instantiation time, so does not really matter I think?
this.args.visible and this._visible are the samantically the same, except that _visible can diverge (see the other comment re controlled vs. uncontrolled), which we can revisit as a breaking API change, but not now. IIRC hidden was needed due to CSS transitions, as the outer element that has transitions defined needs to be present in DOM before the transition is triggered. But tbh I was also quite a bit confused about this when I started this refactoring...
There was a problem hiding this comment.
this.args.visible and this._visible are the samantically the same, except that _visible can diverge
Using a state, which can diverge from the passed in for a default value is confusing for me. Would it be okay for you to use this.visible instead?
| hidden = !this._visible; | |
| hidden = !this.visible; |
| }, | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
I'm not getting how this is related to a transition. It seems to guard against fastboot only. It seems to be a @unlessFastboot decorator.
There was a problem hiding this comment.
It's not really new, we had it already as a CP macro here: https://github.com/kaliber5/ember-bootstrap/blob/master/addon/utils/cp/uses-transition.js
It does check for this.args.fade, so not really just about FastBoot though!?
There was a problem hiding this comment.
It does check for
this.args.fade, so not really just about FastBoot though!?
Maybe I totally missed something but it doesn't seem to check this.args.fade but a dynamic argument passed in. The only relationship to fade, which I can see is the variable name fadeProperty.
But I agree that we can revisit this later as it was like this before.
simonihmig
force-pushed
the
glimmer-alert
branch
from
2558022
to
2900210
Compare
Done. Only for you! 😉 |
No description provided.