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.

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(material/select): add page down/up button functionality #25508

Merged

Conversation

forsti0506
Copy link
Contributor

Hello,

small PR on adding some functionality according to (#3549) and implemented to fit the standard.

Best regards,

Martin

@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch 4 times, most recently from 66cc5a4 to 55f3e82 Compare August 21, 2022 15:29
@@ -280,6 +282,22 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
return;
}

case PAGE_UP:
if (isModifierAllowed) {
this.setFirstItemActive();
Copy link
Member

Choose a reason for hiding this comment

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

Page up/down aren't supposed to focus the first/last items, but rather move one page up/down in the list. The reason why we haven't implemented it in the ListKeyManager is that it doesn't know how large the list is and the offset at which focus is set currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, if you take a look at my W3C link, it is possible to go 10 elements down/up or select the first/last option for the implentation of page_up/page_down. Or did I udnerstand somethin wrong from this reference implementation?

Copy link
Member

Choose a reason for hiding this comment

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

The number 10 seems a little arbitrary to me, my understanding is that at least in native applications, it's supposed to go up/down by a page (hence the name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitly understand your concerns. I think the idea behing is, that a combobox is an "popup" which is after opening handled like it is a own page => therefore the jump of 10 (or last/first).

If you like I can implement the jump for 10 items, or we can forget the PR. I only saw it is an easy fix and thought i contribute it :)

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine, but we should make it configurable, similar to how it's done for the home/end keys. E.g. something with ListKeyManager.withPageUpDown(10).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed some tests and the integration with jumping 10 or to first/last one! Are there sth missing?

@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch from 2af210e to 3e1d4f2 Compare August 25, 2022 20:58
@forsti0506 forsti0506 requested review from crisbeto and removed request for jelbourn and devversion September 15, 2022 18:14
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

The CI is failing, because:

  1. The API goldens need to be updated. You can do it by running yarn approve-api a11y and committing the changed file.
  2. There's a merge commit which is tripping up the commit message validator. Usually we rebase branches to avoid merge commits.

case PAGE_UP:
if (this._pageUpAndDown.enabled && isModifierAllowed) {
const targetIndex = this._activeItemIndex - this._pageUpAndDown.delta;
this._setActiveItemByIndex(targetIndex > 0 ? targetIndex : 0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to simplify this and the PAGE_DOWN handling using the _setActiveItemByDelta method instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During writing the code I first had the same intention, but in my opinion this method can only be used to move 1 up or down and not an defined number or did i misunderstand anything?

Copy link
Member

Choose a reason for hiding this comment

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

I see. Let's keep it like this for now.

@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch from 791051e to 24a2143 Compare September 16, 2022 20:20
@angular-robot angular-robot bot added flag: breaking change area: docs Related to the documentation labels Sep 16, 2022
@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch from 1effeda to 24a2143 Compare September 16, 2022 20:53
@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch from 24a2143 to a50fb0b Compare September 16, 2022 20:56
@forsti0506
Copy link
Contributor Author

forsti0506 commented Sep 16, 2022

The CI is failing, because:

1. The API goldens need to be updated. You can do it by running `yarn approve-api a11y` and committing the changed file.

2. There's a merge commit which is tripping up the commit message validator. Usually we rebase branches to avoid merge commits.
  1. file is updated
  2. I did the rebase!

@forsti0506 forsti0506 force-pushed the mat-select-Support-PageUp-and-PageDown branch from a50fb0b to c181fd8 Compare September 16, 2022 21:04
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

case PAGE_UP:
if (this._pageUpAndDown.enabled && isModifierAllowed) {
const targetIndex = this._activeItemIndex - this._pageUpAndDown.delta;
this._setActiveItemByIndex(targetIndex > 0 ? targetIndex : 0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

I see. Let's keep it like this for now.

@crisbeto crisbeto changed the title feat(mat/select): add page down/up button functionality feat(material/select): add page down/up button functionality Sep 17, 2022
@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed area: docs Related to the documentation labels Sep 17, 2022
@forsti0506
Copy link
Contributor Author

LGTM

Nice, cu soon hopefully :)

@andrewseguin andrewseguin self-assigned this Sep 22, 2022
@andrewseguin andrewseguin merged commit 8ca3155 into angular:main Sep 22, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants