feat(dialog, side-drawer): add cancel event (VIV-1899) #1869
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1869 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 123 350 +227
Lines 1562 6840 +5278
Branches 108 887 +779
===========================================
+ Hits 1562 6840 +5278
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
RichardHelm
force-pushed
the
VIV-1899-cancel-event
branch
from
4728999 to
263f738
Compare
| @@ -72,7 +65,7 @@ export const DialogTemplate: ( | |||
| return html<Dialog>` | |||
| <${elevationTag} dp="8"> | |||
| <dialog class="${getClasses}" | |||
| @keydown="${(x, c) => handleEscapeKey(x, c.event)}" | |||
| @keydown="${(x, c) => x._onKeyDown(c.event as KeyboardEvent)}" | |||
There was a problem hiding this comment.
Please remove _onKeyDown from the class and create a local template function.
There was a problem hiding this comment.
As with the others, I think all logic should go into the class
There was a problem hiding this comment.
After we saw how things can go wrong in the Dialog, I think we can safely revert to our own coding standards and remove the _ to prevent bad interface leaks to production. WDYT?
There was a problem hiding this comment.
Mhm, I still think it's best to have the method on the class so I'm unwilling to change it.
It would be confusing if some methods are on the class and some defined in the template. The only reason to do so is to avoid _, but as you know, I don't see a problem with it
Co-authored-by: Yonatan Kra <yonatan.kra@vonage.com>
No description provided.