feat(ngx-inform): Add modal setup #219
Conversation
IbenTesara
added
enhancement
libs/inform/src/lib/abstracts/modal/modal.abstract.component.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * A wrapper service to Angular CDK Dialog that provides a WCAG/ARIA compliant implementation of modals | ||
| * | ||
| * @export |
There was a problem hiding this comment.
Why document the export? I don't think this is something we've been doing up until now, what are the benefits?
There was a problem hiding this comment.
It's been a thing we've started doing in the VLAIO project a while back and it is in line with what Angular does for their services as well.
Other than extra information I don't think there's a major benefit, but I don't see a downside either.
| // Iben: Return the modal action | ||
| return combineLatest([ | ||
| // Iben: Set the start value to undefined so both actions at least emit once | ||
| modal.action.pipe(startWith(undefined)), |
There was a problem hiding this comment.
I think we've discussed this in the past, but just noting that using undefined is only considerd when using ES5+ in strict mode. If not, undefined can be overwritten and this can cause unwanted behaviour.
(Also applies to several other places in this PR)
There was a problem hiding this comment.
I am aware, but given that this is just internal use I don't see an issue here in this specific spot? Unless you can point me to a potential issue here?
For me it's the same as using first vs take(1) in rjxs. As long as you know the difference and you use the correct one in the correct situation, I'm fine with either one if you use it in a situation where they're interchangeable.
IbenTesara
force-pushed
the
feat/ngx-inform/modal
branch
from
e4dfde8 to
30cadcb
Compare
IbenTesara
force-pushed
the
feat/ngx-inform/modal
branch
from
30cadcb to
76a8eb8
Compare
WHeirstrate
force-pushed
the
feat/ngx-inform/modal
branch
from
76a8eb8 to
be55aef
Compare
Adds a forced WCAG/ARIA compliant wrapper around the CDK Dialog.