lyne-components icon indicating copy to clipboard operation
lyne-components copied to clipboard

refactor: create base class for overlay functionality

Open DavideMininni-Fincons opened this issue 1 year ago • 5 comments

Two points for discussion which possibly need fix:

  • naming: SbbDialogBaseElement uses 'dialog' for naming props (dialogRefs, dialogController), which can result not totally correct in SbbOverlayElement, which uses 'overlay' keyword (_overlayContentElement). Adding a SbbOverlayBase class (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 as public 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 SbbOverlayCloseEventDetails has been renamed to SbbDialogCloseEventDetails

DavideMininni-Fincons avatar Apr 18 '24 10:04 DavideMininni-Fincons

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.

codecov-commenter avatar Apr 18 '24 10:04 codecov-commenter

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

DavideMininni-Fincons avatar May 06 '24 07:05 DavideMininni-Fincons

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

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.

jeripeierSBB avatar May 06 '24 07:05 jeripeierSBB

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

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.

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

DavideMininni-Fincons avatar May 06 '24 07:05 DavideMininni-Fincons

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

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.

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

TomMenga avatar May 07 '24 12:05 TomMenga