lyne-components
lyne-components copied to clipboard
refactor: create base class for overlay functionality
Two points for discussion which possibly need fix:
-
naming:
SbbDialogBaseElementuses 'dialog' for naming props (dialogRefs,dialogController), which can result not totally correct inSbbOverlayElement, which uses 'overlay' keyword (_overlayContentElement). Adding aSbbOverlayBaseclass (which could be extended also by menu, navigation, etc) increases the noise; -
lint issue: the dialog base class declares
public static readonly events = { ... }which is common between the dialog and the overlay; however if in the overlay is overridden aspublic static readonly override events = { ...super.event, backClick: 'requestBackAction' }, the 'missing-component-documentation-rule' breaks, because it expects only properties and not a spread operator with super calls (fixable by #2600). edit: forgot to tag @kyubisation @jeripeierSBB
BREAKING CHANGE
- type
SbbOverlayCloseEventDetailshas been renamed toSbbDialogCloseEventDetails
Codecov Report
Attention: Patch coverage is 98.68421% with 2 lines in your changes are missing coverage. Please review.
Project coverage is 93.24%. Comparing base (
e6397df) to head (6dd1bcb). Report is 67 commits behind head on main.
:exclamation: Current head 6dd1bcb differs from pull request most recent head fb3ddfb. Consider uploading reports for the commit fb3ddfb to get more accurate results
| Files | Patch % | Lines |
|---|---|---|
| src/components/dialog/dialog/dialog.ts | 99.03% | 1 Missing :warning: |
| src/components/overlay/overlay.ts | 97.82% | 1 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## main #2599 +/- ##
==========================================
+ Coverage 93.18% 93.24% +0.05%
==========================================
Files 316 318 +2
Lines 25389 25005 -384
Branches 2063 2037 -26
==========================================
- Hits 23660 23316 -344
+ Misses 1696 1656 -40
Partials 33 33
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
There is an issue with the generation of the custom-elements.json, and then of the docs.
The notification component has a private open method.
If I set the open and close methods as protected in the OverlayBaseClass and then I override it as public in all the components except the notification open, the change is not reflected and all the methods disappear from the readmes.
If I set the open and close methods as public in the OverlayBaseClass and then I add @internal in the open method in the notification to hide it, the internal is not taken into account and the method is visible in the readme.
Should i open a bug to https://github.com/webcomponents/custom-elements-manifest ?
edit: also the selection panel has private open/close methods, and has the same issue
There is an issue with the generation of the custom-elements.json, and then of the docs.
The notification component has a
privateopen method. If I set theopenandclosemethods as protected in the OverlayBaseClass and then I override it aspublicin all the components except the notificationopen, the change is not reflected and all the methods disappear from the readmes. If I set theopenandclosemethods as public in the OverlayBaseClass and then I add@internalin theopenmethod in the notification to hide it, the internal is not taken into account and the method is visible in the readme.Should i open a bug to https://github.com/webcomponents/custom-elements-manifest ?
edit: also the selection panel has private open/close methods, and has the same issue
Should we maybe exclude them from inheriting the base class? Opening a bug report could anyways help. but potentially they have the same rule as for inheriting itself: public props cannot be made private.
There is an issue with the generation of the custom-elements.json, and then of the docs. The notification component has a
privateopen method. If I set theopenandclosemethods as protected in the OverlayBaseClass and then I override it aspublicin all the components except the notificationopen, the change is not reflected and all the methods disappear from the readmes. If I set theopenandclosemethods as public in the OverlayBaseClass and then I add@internalin theopenmethod in the notification to hide it, the internal is not taken into account and the method is visible in the readme. Should i open a bug to https://github.com/webcomponents/custom-elements-manifest ? edit: also the selection panel has private open/close methods, and has the same issueShould we maybe exclude them from inheriting the base class? Opening a bug report could anyways help. but potentially they have the same rule as for inheriting itself: public props cannot be made private.
good, i'll exclude them for now and add a note
i would expect that base protected => derived public should work :'(
add issue: https://github.com/open-wc/custom-elements-manifest/issues/253
There is an issue with the generation of the custom-elements.json, and then of the docs. The notification component has a
privateopen method. If I set theopenandclosemethods as protected in the OverlayBaseClass and then I override it aspublicin all the components except the notificationopen, the change is not reflected and all the methods disappear from the readmes. If I set theopenandclosemethods as public in the OverlayBaseClass and then I add@internalin theopenmethod in the notification to hide it, the internal is not taken into account and the method is visible in the readme. Should i open a bug to https://github.com/webcomponents/custom-elements-manifest ? edit: also the selection panel has private open/close methods, and has the same issueShould we maybe exclude them from inheriting the base class? Opening a bug report could anyways help. but potentially they have the same rule as for inheriting itself: public props cannot be made private.
good, i'll exclude them for now and add a note
i would expect that base protected => derived public should work :'(
add issue: open-wc/custom-elements-manifest#253
Another option is to remove the open/close abstract method from the base class (since it's not explicitly needed).
That way, you can use the base class for the notification and the expansion-panel