Datagrid filter setFocus #542
Conversation
The property `filterContainer` is used by the subclasses and is gonna be needed for this base class also so we make it protected. Signed-off-by: Ivo Rahov <[email protected]>
Changes include: - added abstract method setFocus -added implementation for the different filters to set focus Signed-off-by: Ivo Rahov <[email protected]>
| @@ -127,6 +127,10 @@ export class DatagridMultiSelectFilterComponent extends DatagridFilter<string[], | |||
| !!Object.keys(this.formGroup.getRawValue()).filter((frmCtrl) => this.formGroup.get(frmCtrl).value).length | |||
| ); | |||
| } | |||
|
|
|||
| protected setFocus() { | |||
There was a problem hiding this comment.
I think a better approach is to have a method called getFocusableElement(), then the callers just have to return the element instead of every implementation calling focus().
There was a problem hiding this comment.
I like the focus() since it seems a more complete approach to me.
If I come accross to getFocusableElement() function - it is not that clear how the returned element will be used. Yes, the name suggests to focus, but yet - not that clean. In addition, I'm not sure if there is a case when something else may be needed when focusing the element. On the other hand focus() is more concise and clear.
I agree that the very .focus() call can be reused in the base class but I don't see it has the advantages as the other approach.
| protected constructor(filterContainer: ClrDatagridFilter, private subscriptionTracker: SubscriptionTracker) { | ||
| protected constructor( | ||
| protected filterContainer: ClrDatagridFilter, | ||
| private subscriptionTracker: SubscriptionTracker |
There was a problem hiding this comment.
Since we're touching this file, we may as well remove the providers from here since it doesn't have any effect, the concrete classes need to add the providers.
There was a problem hiding this comment.
@kepelrs discovered this while analyzing the SubscriptionTracker
There was a problem hiding this comment.
IIRC it was @irahov who discovered it first :)
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What does this change do?
When opening a datagrid filter the focus should go directly to the relevant HTML element so that the user can start filtering right away.
Up to now the focus was on the flter close button 'X'. This is extremely boring when using string filter because the user has to click first on the input field (or TAB to it) and then start writing.
What manual testing did you do?
In the example applications navigate to
Datagrid->Datagrid Filters(/datagrid/example/datagrid-filter)Open each of the filters and verify that the focus is on the
inputfield or theselect.Screenshots (if applicable)
Does this PR introduce a breaking change?
Breaking changes need:
filterContainer: ClrDatagridFilteronlyOther information