refactor: remove link-button properties and create separate classes

[DavideMininni-Fincons]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the pull request tracker for a Pull Request (PR) that matches the one I want to submit, without success.

Issue

This PR Closes #2212

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

See Review Guidelines for more information what is checked during review process.

Changes

Changes in this pull request:

• A new class named SbbActionBaseElement has been created. This is used as a base class for all the button/link related components; in particular, the renderTemplate method must be implemented by all the derived classes, returning the inner template to render.

• Two other base class have been created, named SbbButtonBaseElement and SbbLinkBaseElement, which both extends the SbbActionBaseElement class, and respectively implement the ButtonProperties and the LinkProperties interfaces. These two classes contains all the logic (attributes, interaction, render) related to the natives button and link re-implementation.

• Four new mixins have been created, which permits adding the negative, disabled and icon behaviors to any custom component, plus a disabled mixin customized for action elements which also handles the tabindex and aria-disabled attribute's setting. The mixins have been used to replace the properties functionality wherever possible:

  // old code
  export class SbbAutocompleteElement extends SlotChildObserver(LitElement) {
     ...
     /** Negative coloring variant flag. */
     @property({ reflect: true, type: Boolean }) public negative = false;
     ...
  }
  
  // new code
  export class SbbAutocompleteElement extends SlotChildObserver(SbbNegativeMixin(LitElement)) {
     ...
  }

• Each of the following component, which previously implemented the LinkButtonInterface, has been split in two separate components; the first one extends the SbbButtonBaseElement, the second one extends the SbbLinkBaseElement. The extension has been done by creating another mixin, generally named <Component>CommonElementMixin, which contains the shared logic (additional properties, styles, (dis)connectedCallback() and so on).

  // old code
  @customElement('sbb-header-action')
  export class SbbHeaderActionElement extends LitElement implements LinkButtonProperties {
    ...
  }
  
  // new code
  @customElement('sbb-header-button')
  export class SbbHeaderButtonElement extends SbbHeaderActionCommonElementMixin(SbbButtonBaseElement) {}
  
  @customElement('sbb-header-link')
  export class SbbHeaderLinkElement extends SbbHeaderActionCommonElementMixin(SbbLinkBaseElement) {}

  Affected components are:

  • sbb-button: split in sbb-button and sbb-button-link;
  • sbb-card-action: split in sbb-card-button and sbb-card-link;
  • sbb-link: split in sbb-link-button and sbb-link;
  • sbb-menu-action: split in sbb-menu-button and sbb-menu-link;
  • sbb-navigation-action: split in sbb-navigation-button and sbb-navigation-link.

• The isStatic interface has been removed from codebase; for the button and the link, two new other components, named sbb-button-static and sbb-link-static, have been created to replace the button/link's isStatic functionality. The check for nested buttons / links has been removed too, so now consumers must check these cases, and possibly fix them using the two new static variants.

• The static functionality is no longer available also in sbb-breadcrumb, sbb-teaser, sbb-teaser-hero.

• Apart from the link-button case, these components have been extended from base class:

  • sbb-breadcrumb now extends SbbLinkBaseElement;
  • sbb-datepicker-next-day now extends SbbButtonBaseElement;
  • sbb-datepicker-previous-day now extends SbbButtonBaseElement;
  • sbb-expansion-panel-header now extends SbbButtonBaseElement;
  • sbb-form-field-clear now extends SbbButtonBaseElement;
  • sbb-tag now extends SbbButtonBaseElement;
  • sbb-teaser-hero now extends SbbLinkBaseElement;
  • sbb-teaser now extends SbbLinkBaseElement;
  • sbb-tooltip-trigger now extends SbbButtonBaseElement;

Browsers

I tested the build on the following browsers:

• Firefox Desktop
• Chrome Desktop
• Edge Desktop
• Safari Desktop
• Chrome Mobile
• Safari Mobile

Screen readers

I tested the build on the following browsers:

• JAWS Firefox Desktop
• JAWS Chrome Desktop
• NVDA Firefox Desktop
• NVDA Chrome Desktop
• VoiceOver Safari Desktop
• VoiceOver Chrome Desktop
• VoiceOver Safari Mobile
• Android Accessibility Suite Chrome Mobile

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

Does this introduce a breaking change?

• Yes
• No

BREAKING CHANGES:
All of these components has been split in two, one with only-button and one with only-link behavior.

• sbb-card-action: split in sbb-card-button and sbb-card-link;
• sbb-menu-action: split in sbb-menu-button and sbb-menu-link;
• sbb-navigation-action: split in sbb-navigation-button and sbb-navigation-link.

These components has been split in three, one with only-button and one with only-link behavior, plus a static variant.

• sbb-button: split in sbb-button, sbb-button-link and sbb-button-static;
• sbb-link: split in sbb-link, sbb-link-button and sbb-link-static.

Check your code and change accordingly, eg.

• sbb-button with href set => change to sbb-button-link;
• sbb-menu-action with name and form set => change to sbb-menu-button;
• sbb-link with href set => no changes;
• sbb-navigation-action with value set => change to sbb-navigation-button;
• sbb-button with isStatic set to true => change to sbb-button-static;
• sbb-link used within an anchor tag => change to sbb-link-static;
• ...

Other information

[DavideMininni-Fincons]

Hello @kyubisation
as mentioned some dailies ago, there's a problem with the react build.

I receive this error (on navigation, but time by time also on menu, card, etc):

Could not resolve "./navigation-action" from "src/react/navigation/index.ts"
file: /Users/davide.mininni/workspace/workspace_lyne/lyne-components/src/react/navigation/index.ts
error during build:
RollupError: Could not resolve "./navigation-action" from "src/react/navigation/index.ts"
    at error (file:///Users/davide.mininni/workspace/workspace_lyne/lyne-components/node_modules/rollup/dist/es/shared/parseAst.js:337:30)
    at ModuleLoader.handleInvalidResolvedId (file:///Users/davide.mininni/workspace/workspace_lyne/lyne-components/node_modules/rollup/dist/es/shared/node-entry.js:18022:24)
    at file:///Users/davide.mininni/workspace/workspace_lyne/lyne-components/node_modules/rollup/dist/es/shared/node-entry.js:17982:26
error Command failed with exit code 1.

I added a new index.ts file in the -action folder, which exports both the newly created button and link component; then I import it in the main index.ts.

from my understanding of the src/react/vite.config.ts, I think this interfere with the dirInfo setting at line 110.
I tried to remove this new index, refactoring imports, but the error persists.
Any idea?

[kyubisation]

I'll try to have a look.
Without having done a review; I think I would prefer the following structure for previous action elements:
e.g.

• menu
  • common
  • menu-button
  • menu-link
  • menu

i.e. a flatter structure then nesting the action elements in menu/menu-action.

@jeripeierSBB @DavideMininni-Fincons What do you think?

[DavideMininni-Fincons]

I'll try to have a look. Without having done a review; I think I would prefer the following structure for previous action elements: e.g.

• menu

  • common
  • menu-button
  • menu-link
  • menu

i.e. a flatter structure then nesting the action elements in menu/menu-action.

@jeripeierSBB @DavideMininni-Fincons What do you think?

I tried it and it solved the react build problem too, so even if i don't really like it due the position of the common folder, it's fine for me (I'm pushing the fix in a few moments).

edited: i forgot to write, but for sure there are still stuff to fix (eg, i created some new mixin but not sure about them, maybe can we merge common styles?, there are comments about migr hostAttributes, and so on), so any other addition/improvement is well accepted :)

[codecov-commenter]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@34d3dbb). Click here to learn what that means.

Files	Patch %	Lines
src/components/menu/menu/menu.ts	81.25%	6 Missing ⚠️
src/components/teaser-hero/teaser-hero.ts	92.59%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2364   +/-   ##
=======================================
  Coverage        ?   93.55%           
=======================================
  Files           ?      219           
  Lines           ?    22098           
  Branches        ?     2006           
=======================================
  Hits            ?    20674           
  Misses          ?     1389           
  Partials        ?       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DavideMininni-Fincons]

An example of how to use the new disabled and negative mixins has been done here:
https://github.com/lyne-design-system/lyne-components/compare/refactor/link-button-components-refactoring...refactor/link-button-components-refactoring-applying-mixins?expand=1

After the merge of this PR, a new PR from the 'refactor/link-button-components-refactoring-applying-mixins' branch can be created.

[kyubisation]

Fantastic work!