-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: sbb autocomplete grid #2512
Conversation
2600e46
to
ed2b24b
Compare
Codecov ReportAttention: Patch coverage is
❗ 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. |
15d25eb
to
a8cc8c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job as usual
Only a partial review ( around half of it ) 😵💫
src/components/autocomplete-grid/autocomplete-grid-actions/autocomplete-grid-actions.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-actions/autocomplete-grid-actions.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-option/autocomplete-grid-option.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-option/autocomplete-grid-option.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-row/autocomplete-grid-row.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid-row/autocomplete-grid-row.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts
Outdated
Show resolved
Hide resolved
1276487
to
673be75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, few things to fix
src/components/autocomplete-grid/autocomplete-grid-option/autocomplete-grid-option.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts
Outdated
Show resolved
Hide resolved
src/components/autocomplete-grid/autocomplete-grid/autocomplete-grid.spec.ts
Outdated
Show resolved
Hide resolved
8dedb4f
to
5985077
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Note for reviewers: 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 😃
Some minor nitpicks, but there is an issue with the mini button base.
After that it should be good to merge!
src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts
Outdated
Show resolved
Hide resolved
src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts
Outdated
Show resolved
Hide resolved
src/elements/autocomplete-grid/autocomplete-grid-button/autocomplete-grid-button.ts
Outdated
Show resolved
Hide resolved
src/elements/autocomplete-grid/autocomplete-grid-optgroup/autocomplete-grid-optgroup.ts
Outdated
Show resolved
Hide resolved
src/elements/autocomplete-grid/autocomplete-grid-optgroup/readme.md
Outdated
Show resolved
Hide resolved
…-grid # Conflicts: # src/elements/datepicker/datepicker/datepicker.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thank you very much! 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Congratulations, hard work done!
Preflight Checklist
Issue
This PR Closes #1945
Pull request checklist
Please check if your PR fulfills the following requirements:
See Review Guidelines for more information what is checked during review process.
Changes
Changes in this pull request:
sbb-autocomplete-grid
component added, with related components:sbb-autocomplete-grid-optgroup
, similar to native optgroupsbb-autocomplete-grid-row
, which acts as a container for both option and actionssbb-autocomplete-grid-option
, similar to native optionsbb-autocomplete-grid-actions
, which acts as a container for buttonsbb-autocomplete-grid-button
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?
Other information