-
Notifications
You must be signed in to change notification settings - Fork 133
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
fix: update combobox with latest fundamental-styles #2188
Conversation
Deploy preview for fundamental-ngx ready! Built with commit 4338b98 |
* The object attribute by which to group items. | ||
*/ | ||
@Input() | ||
groupBy: string; |
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.
I know we had converstaion about this idea, but maybe groupFunction
will be better there. I just realised that there can be more complex/indent objects.
apps/docs/src/app/core/component-docs/combobox/examples/combobox-columns-example.component.ts
Show resolved
Hide resolved
apps/docs/src/app/core/component-docs/combobox/examples/combobox-group-example.component.html
Outdated
Show resolved
Hide resolved
apps/docs/src/app/core/component-docs/combobox/examples/combobox-group-example.component.ts
Outdated
Show resolved
Hide resolved
apps/docs/src/app/core/component-docs/combobox/examples/combobox-group-example.component.ts
Outdated
Show resolved
Hide resolved
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.
Could you make sure that:
- all methods in
libs/core/src/lib/combobox/combobox.component.ts
have correct typings - all
private
properties have correct name convention (_ prefix)
I would be also great if:
- you move code currently placed inside component life-cycle hooks to separate methods.
- you could wait until we merge feat: Button in FIori3 | Dialog in Fiori3 | New way of adding Icons #2224 so you can use fdTemplate for providing custom templates instead of
@Input
LGTM @salarenko , could you take a look at the PR again and submit a new review? Thanks :) |
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.
Except that little console.log
code looks fine. One thing that bothers me are differences in behavior between our Combobox implementation and SAPUI5 Combobox and in my opinion SAPUI5 makes more sense.
NGX Core Combobox vs SAPUI5 Combobox behaviour:
Action | NGX Core | SAPUI5 |
---|---|---|
Focus input with Tab and start typing |
No action | Displays list of elements matching provided input. |
Click on Combobox input |
Displays list of all options | No action |
Click on Combobox input and start typing |
Filters elements lists, makrs list element only when its 1:1 with typed in query, element cannot be selected without manual Arrow Down navigation and Enter selection. |
Filters elements lists, marks most accurate result on the list which can be selected by pressing Enter key. |
Select an element from the list and click on Input expand button |
Shows results filtered by selected element. | Show all possible options with currently selected element focused. |
Click on Combobox input and start typing |
No element has visible focus. | Input is focused. |
Expand options list | No visuals for Combobox input group changed | Expand button marked as active |
I have also a couple of reflections on the documentation:
-
After reading it I do not exactly understand what is the difference between
filterFn
andsearchFunction
I feel like this needs a little more explanation especially onsearchFunction
side. Also API naming convention conflicts here. -
Description of 'Display Object Property' section is also quite hard to understand or maybe just wrong. (I do not understand it)
-
In 'Custom Item Template' you need clearly state that custom template supports List component features and provide link to the component.
-
' Combobox with Two Columns' breaks after selecting list element
} | ||
|
||
groupFunc(items: any): {} { | ||
console.log({ |
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.
console.log and still needs type annotation
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.
@mikerodonnell89 @salarenko has the above been addressed?
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.
As we discussed in scrum on monday, click behavior will be reworked in another issue #2285 |
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
Please provide a link to the associated issue.
#2007
Please provide a brief summary of this pull request.
Update combobox to latest version of fundamental-styles. Currently missing the mobile mode which uses dialog, as ngx still hasn't been updated to dialog.
Before:
After:
Please check whether the PR fulfills the following requirements
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
Documentation checklist:
README.md