Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#119 - Issue with scrollTop and keyboard selection #120

Merged
merged 4 commits into from
Mar 21, 2019
Merged

#119 - Issue with scrollTop and keyboard selection #120

merged 4 commits into from
Mar 21, 2019

Conversation

Mabiro
Copy link

@Mabiro Mabiro commented Mar 18, 2019

-Attempt at fixing the issue with _scrollActiveOptionIntoView when us…ing the ngx-mat-select-search input

Fixes #119

I still had to import _countGroupLabelsBeforeOption, sadly, to make it work with groups.

@Mabiro Mabiro mentioned this pull request Mar 18, 2019
Closed
@Mabiro
Copy link
Author

Mabiro commented Mar 18, 2019

Any alternative to using _getItemHeight?

Mabiro
Mabiro commented Mar 18, 2019
View reviewed changes
src/app/mat-select-search/mat-select-search.component.ts Outdated
@@ -285,6 +285,30 @@ export class MatSelectSearchComponent implements OnInit, OnDestroy, AfterViewIni
this.changeDetectorRef.markForCheck();
});
});

if (this.matSelect._keyManager) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it looks like at this point, _keyManager was sometimes undefined. (It was the case for the app-custom-clear-icon-example component)

Is there a safer place I could subscribe to the event, where I can know for sure that _keyManager is defined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is initialized in the ngAfterContentInit lifecycle hook:
https://github.com/angular/material2/blob/eb5eed14778f402ea56a096148ff6e78d8bdad93/src/lib/select/select.ts#L542-L543

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should initialize first time after the mat select is opened (See #54):

ngx-mat-select-search/src/app/mat-select-search/mat-select-search.component.ts

Lines 226 to 232 in 4b92230

// set the first item active after the options changed
this.matSelect.openedChange
.pipe(take(1))
.pipe(takeUntil(this._onDestroy))
.subscribe(() => {
this._options = this.matSelect.options;
this._options.changes

however, i would keep the if (this.matSelect._keyManager) { and maybe add a console.log() to avoid errors if in some circumstances the keymanager is not initialized yet. See e.g. #53

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the subscribtion and added the console.log.

However, I added a placeholder message for now, let me know what you think it should include.

@macjohnny
Copy link
Member

@Mabiro thanks for your contribution. I will soon have a closer look

macjohnny
macjohnny reviewed Mar 19, 2019
View reviewed changes
src/app/mat-select-search/mat-select-search.component.ts Outdated
@@ -285,6 +285,30 @@ export class MatSelectSearchComponent implements OnInit, OnDestroy, AfterViewIni
this.changeDetectorRef.markForCheck();
});
});

if (this.matSelect._keyManager) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move your implementation into a separate function? just to make it easier to read...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, sorry about that!

@Mabiro
Copy link
Author

Mabiro commented Mar 19, 2019 •
edited
Loading

@macjohnny : I am still having the _getItemHeight being a private method issue.

I could use ['_getItemHeight'](), but I am not sure I should.

-Moved scrollTop adjustement in a function
ca51d41 
-Moved the location of keyManager.changes subscription to a later lifecycle
@macjohnny
Copy link
Member

@macjohnny : I am still having the _getItemHeight being a private method issue.

I could use ['_getItemHeight'](), but I am not sure I should.

wouldn't it be possible to get the height calculated in the dom with getBoundingClientRect() ?

@Mabiro
Copy link
Author

Mabiro commented Mar 19, 2019

@macjohnny : I am still having the _getItemHeight being a private method issue.
I could use ['_getItemHeight'](), but I am not sure I should.

wouldn't it be possible to get the height calculated in the dom with getBoundingClientRect() ?

I'll give it a shot soon-ish!

macjohnny
macjohnny approved these changes Mar 20, 2019
View reviewed changes
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
it works well. we should also call adjustScrollTopToFitActiveOptionIntoView() when the options change to make sure the hightlighted item is visible
thanks a lot for your contribution!

@Mabiro
Copy link
Author

Mabiro commented Mar 20, 2019

@macjohnny I added adjustScrollTopToFitActiveOptionIntoView on options change :)

@macjohnny macjohnny merged commit 3629a2e into bithost-gmbh:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macjohnny macjohnny macjohnny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] When using down or up arrow, the mat-select scrollTop isn't calculated correctly
2 participants
@Mabiro @macjohnny