feat: sbb autocomplete grid #2512

DavideMininni-Fincons · 2024-03-21T09:14:55Z

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 #1945

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:

new sbb-autocomplete-grid component added, with related components:
- sbb-autocomplete-grid-optgroup, similar to native optgroup
- sbb-autocomplete-grid-row, which acts as a container for both option and actions
- sbb-autocomplete-grid-option, similar to native option
- sbb-autocomplete-grid-actions, which acts as a container for button
- sbb-autocomplete-grid-button
the autocomplete, optgroup and option components have been refactored using a base class

Browsers

I tested the build on the following browsers:

Screen readers

I tested the build on the following browsers:

Pull request type

Please check the type of change your PR introduces:

Does this introduce a breaking change?

Yes
No

Other information

codecov-commenter · 2024-03-22T08:39:12Z

Codecov Report

Attention: Patch coverage is 98.09524% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.24%. Comparing base (e6397df) to head (ee4d5ec).
Report is 66 commits behind head on main.

❗ Current head ee4d5ec differs from pull request most recent head b77607c. Consider uploading reports for the commit b77607c to get more accurate results

Files	Patch %	Lines
src/components/autocomplete/autocomplete.ts	92.85%	2 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2512      +/-   ##
==========================================
+ Coverage   93.18%   93.24%   +0.05%     
==========================================
  Files         316      316              
  Lines       25389    24613     -776     
  Branches     2063     1986      -77     
==========================================
- Hits        23660    22950     -710     
+ Misses       1696     1631      -65     
+ Partials       33       32       -1

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

TomMenga

Good job as usual
Only a partial review ( around half of it ) 😵‍💫

src/components/autocomplete-grid/autocomplete-grid-actions/autocomplete-grid-actions.spec.ts

src/components/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.spec.ts

src/components/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts

src/components/autocomplete-grid/autocomplete-grid-option/autocomplete-grid-option.ts

src/components/autocomplete-grid/autocomplete-grid-row/autocomplete-grid-row.spec.ts

src/components/autocomplete-grid/autocomplete-grid-row/autocomplete-grid-row.ts

src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts

TomMenga

Hello, few things to fix

src/components/autocomplete-grid/autocomplete-grid-option/autocomplete-grid-option.spec.ts

src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts

TomMenga

LGTM 👍

…-grid # Conflicts: # src/elements/autocomplete/autocomplete.spec.ts # src/elements/button/mini-button/mini-button.ts # src/elements/option/optgroup/optgroup.ts # src/elements/option/option/option.ts

…-grid

DavideMininni-Fincons · 2024-07-04T15:11:10Z

Note for reviewers:
apparently, the autocomplete behavior (add bold style to options that match the typed text) was not working anymore due some changes related to SSR.

A fix was made in the commit 'fix: enable autocomplete behavior' 89538b1 but since i'm not fully aware of what exactly is rendered in SSR, the change should be reviewed.

kyubisation

Nice work! 😃
Some minor nitpicks, but there is an issue with the mini button base.
After that it should be good to merge!

kyubisation · 2024-07-05T12:24:56Z

src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts

+  );
+
+  /** Observe changes on data attributes and set the appropriate values. */
+  private _onOptionAttributesChange(mutationsList: MutationRecord[]): void {


Nitpick; This could probably be inlined, as it is only used in the observer.

kyubisation · 2024-07-05T12:25:34Z

src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts

+
+  public constructor() {
+    super();
+    new SbbSlotStateController(this);


Nitpick; You could now use the slotState decorator.

kyubisation · 2024-07-05T13:04:59Z

src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts

+  /**
+   * Used to dispatch a click event when users interact with the button via keyboard (the component does not receive focus).
+   */
+  public dispatchClick(event: KeyboardEvent): void {


I'm not sure whether this needs to be a public method on this class. Could we not just dispatch a click event from the autocomplete grid, where this method is called from?

As usual, the click on the ac-grid-button emits a click event with the button component as a target;
I could move this method in the ac-grid, however this will cause to emit a forced click event on keyboard selection with the ac-grid as a target, even if the 'visual-focus' is on the button

I'm not sure I fully understand.
My suggestion would be to move the dispatch click event logic to the grid (or somewhere re-usable?), but still dispatch the event on the grid button in the selection via keyboard scenario.
Could you elaborate again?

kyubisation · 2024-07-05T13:22:16Z

src/elements/autocomplete-grid/autocomplete-grid-optgroup/autocomplete-grid-optgroup.ts

+    ) as SbbAutocompleteGridOptionElement[];
+  }
+
+  private get _buttons(): SbbAutocompleteGridButtonElement[] {


Nitpick; Could be inlined.

kyubisation · 2024-07-05T13:23:58Z

src/elements/autocomplete-grid/autocomplete-grid-optgroup/readme.md

@@ -0,0 +1,78 @@
+The `sbb-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)


Suggested change

The `sbb-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)

The `sbb-autocomplete-grid-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)

kyubisation · 2024-07-05T14:25:43Z

src/elements/core/base-elements/button-base-element.ts

    }
  };

-  private _dispatchClickEvent = (event: KeyboardEvent): void => {
+  protected dispatchClickEvent = (event: KeyboardEvent): void => {


This change is no longer needed. If this functionality is needed externally, we can extract it as a function or as a static method.

kyubisation · 2024-07-05T14:27:20Z

src/elements/core/base-elements/mini-button-base-element.ts

+
+@slotState()
+export abstract class SbbMiniButtonBaseElement extends SbbNegativeMixin(
+  SbbIconNameMixin(SbbButtonBaseElement),


We must never import from outside the core directory from inside the core directory.
I would suggest moving this base element to the mini button.
What do you think?

…-grid # Conflicts: # src/elements/datepicker/datepicker/datepicker.ts

DavideMininni-Fincons linked an issue Mar 21, 2024 that may be closed by this pull request

story(sbb-autocomplete-grid): initial implementation #1945

Open

10 tasks

DavideMininni-Fincons force-pushed the feat/sbb-autocomplete-grid branch from 2600e46 to ed2b24b Compare March 21, 2024 09:23

github-actions bot temporarily deployed to preview-pr2512 March 21, 2024 09:33 Inactive

github-actions bot added the preview-available label Mar 21, 2024

DavideMininni-Fincons marked this pull request as ready for review March 21, 2024 10:15

DavideMininni-Fincons requested review from kyubisation and jeripeierSBB as code owners March 21, 2024 10:15

github-actions bot temporarily deployed to preview-pr2512 March 21, 2024 16:11 Inactive

DavideMininni-Fincons force-pushed the feat/sbb-autocomplete-grid branch from 15d25eb to a8cc8c5 Compare March 22, 2024 19:48

github-actions bot temporarily deployed to preview-pr2512 March 25, 2024 09:02 Inactive

github-actions bot temporarily deployed to preview-pr2512 March 25, 2024 16:06 Inactive

github-actions bot temporarily deployed to preview-pr2512 March 26, 2024 08:55 Inactive

github-actions bot temporarily deployed to preview-pr2512 March 26, 2024 10:48 Inactive

DavideMininni-Fincons changed the title ~~feat: sbb autocomplete grid [WIP]~~ feat: sbb autocomplete grid Mar 26, 2024

DavideMininni-Fincons added the pr: peer review required A peer review is required for this pull request label Mar 26, 2024

DavideMininni-Fincons requested review from TomMenga, MarioCastigliano and dauriamarco March 26, 2024 11:31

jeripeierSBB mentioned this pull request Mar 27, 2024

feat(sbb-autocomplete): favourites PoC #1772

Closed

TomMenga requested changes Mar 27, 2024

View reviewed changes

DavideMininni-Fincons force-pushed the feat/sbb-autocomplete-grid branch from 1276487 to 673be75 Compare March 28, 2024 11:26

github-actions bot temporarily deployed to preview-pr2512 March 28, 2024 16:30 Inactive

TomMenga requested changes Apr 3, 2024

View reviewed changes

DavideMininni-Fincons force-pushed the feat/sbb-autocomplete-grid branch from 8dedb4f to 5985077 Compare April 3, 2024 12:41

github-actions bot temporarily deployed to preview-pr2512 April 3, 2024 12:51 Inactive

DavideMininni-Fincons requested a review from TomMenga April 4, 2024 07:11

jeripeierSBB added the pr: lead review required A lead review is required for this pull request label Apr 8, 2024

TomMenga approved these changes Apr 8, 2024

View reviewed changes

TomMenga removed the pr: peer review required A peer review is required for this pull request label Apr 8, 2024

fix: test

0c443db

github-actions bot temporarily deployed to pr2512 June 11, 2024 08:02 Inactive

github-actions bot added pr: visual review required and removed pr: visual review required labels Jun 11, 2024

DavideMininni-Fincons mentioned this pull request Jun 13, 2024

Refactoring: Migration of visual regression tests and testfile restructering #2731

Open

71 tasks

Merge remote-tracking branch 'origin/main' into feat/sbb-autocomplete…

5f69018

…-grid # Conflicts: # src/elements/autocomplete/autocomplete.spec.ts # src/elements/button/mini-button/mini-button.ts # src/elements/option/optgroup/optgroup.ts # src/elements/option/option/option.ts

github-actions bot temporarily deployed to pr2512 July 2, 2024 14:34 Inactive

Merge remote-tracking branch 'origin/main' into feat/sbb-autocomplete…

cfe5e8f

…-grid

github-actions bot temporarily deployed to pr2512 July 3, 2024 09:31 Inactive

github-actions bot added pr: visual review required and removed pr: visual review required labels Jul 3, 2024

test: add visual tests

6903b95

github-actions bot temporarily deployed to pr2512 July 3, 2024 13:16 Inactive

DavideMininni-Fincons added 2 commits July 3, 2024 16:17

test: add visual tests

f17b650

test: add visual tests

aa32139

github-actions bot temporarily deployed to pr2512 July 3, 2024 14:48 Inactive

DavideMininni-Fincons added 5 commits July 3, 2024 17:19

test: add visual tests

7f5c9a8

test: add visual tests

2207343

test: add visual tests

7067373

test: add visual tests

5483591

fix: tests

8211441

github-actions bot temporarily deployed to pr2512 July 4, 2024 12:29 Inactive

DavideMininni-Fincons added 2 commits July 4, 2024 17:01

fix: tests

41e047f

fix: enable autocomplete behavior

89538b1

github-actions bot temporarily deployed to pr2512 July 4, 2024 15:23 Inactive

kyubisation requested changes Jul 5, 2024

View reviewed changes

DavideMininni-Fincons added 2 commits July 8, 2024 10:38

Merge remote-tracking branch 'origin/main' into feat/sbb-autocomplete…

1966a22

…-grid # Conflicts: # src/elements/datepicker/datepicker/datepicker.ts

fix: review (partial)

860b4e9

github-actions bot deployed to pr2512 July 8, 2024 10:00 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sbb autocomplete grid #2512

feat: sbb autocomplete grid #2512

DavideMininni-Fincons commented Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited

Loading

TomMenga left a comment

TomMenga left a comment

TomMenga left a comment

DavideMininni-Fincons commented Jul 4, 2024

kyubisation left a comment •

edited

Loading

kyubisation Jul 5, 2024

kyubisation Jul 5, 2024

kyubisation Jul 5, 2024

DavideMininni-Fincons Jul 8, 2024

kyubisation Jul 8, 2024

kyubisation Jul 5, 2024

kyubisation Jul 5, 2024

kyubisation Jul 5, 2024

kyubisation Jul 5, 2024

		@@ -0,0 +1,78 @@
		The `sbb-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)

	The `sbb-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)
	The `sbb-autocomplete-grid-optgroup` is a component used to group more [sbb-autocomplete-grid-option](/docs/elements-sbb-autocomplete-grid-sbb-autocomplete-grid-option--docs)

feat: sbb autocomplete grid #2512

Are you sure you want to change the base?

feat: sbb autocomplete grid #2512

Conversation

DavideMininni-Fincons commented Mar 21, 2024 • edited Loading

Preflight Checklist

Issue

Pull request checklist

Changes

Browsers

Screen readers

Pull request type

Does this introduce a breaking change?

Other information

codecov-commenter commented Mar 22, 2024 • edited Loading

Codecov Report

TomMenga left a comment

Choose a reason for hiding this comment

TomMenga left a comment

Choose a reason for hiding this comment

TomMenga left a comment

Choose a reason for hiding this comment

DavideMininni-Fincons commented Jul 4, 2024

kyubisation left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideMininni-Fincons commented Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited

Loading

kyubisation left a comment •

edited

Loading