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

feat(material/paginator): add input for configuring the underlying select #13705

Merged
merged 1 commit into from
Feb 26, 2022
Merged

feat(material/paginator): add input for configuring the underlying select #13705

merged 1 commit into from
Feb 26, 2022

Conversation

crisbeto
Copy link
Member

Since we hide the underlying MatSelect inside the MatPaginator, the user doesn't have the ability to configure some of the inputs. These changes introduce an input that proxy some of the supported properties to the select.

Fixes #13646.

@crisbeto crisbeto added the target: minor This PR is targeted for the next minor release label Oct 20, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 20, 2018
@ngbot
Copy link

ngbot bot commented Oct 23, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

jelbourn
jelbourn reviewed Oct 24, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Since it's somewhat related- I had been thinking we might want to change the paginator to use a native <select> by default (with the mat-select being an alternative gated by a different selector). Any thoughts on that approach?

@crisbeto
Copy link
Member Author

I'm not sure I see very many advantages to that, aside from the accessibility, since the select most likely won't have very many options and we'll need to bring in the MatSelectModule anyway if we wanted to support both cases.

@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 36e0b22 to 7531096 Compare October 24, 2018 20:00
@jelbourn
Copy link
Member

Theoretically the mat-select would be tree-shaken if it's not being used, even if the module is present. We'd probably want to confirm that, though.

@crisbeto
Copy link
Member Author

crisbeto commented Oct 24, 2018 •
edited
Loading

The module would have to be added to the imports, otherwise AoT will complain when it compiles the template. AFAIK that's enough to make it non-treeshakeable.

@jelbourn
Copy link
Member

jelbourn commented Dec 4, 2018

Want to run an experiment to see? It would basically be taking an empty CLI app, and then comparing the main bundle size after adding a paginator as it is now, and then alternatively a paginator using the native select.

@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 7531096 to a8f5c3e Compare March 31, 2019 20:22
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguin andrewseguin added the P4 A relatively minor issue that is not relevant to core functions label May 30, 2019
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from a8f5c3e to 9392223 Compare July 6, 2019 10:34
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 9392223 to 7872a1d Compare July 13, 2019 14:57
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 7872a1d to 191d116 Compare September 18, 2019 15:49
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 191d116 to 5458aee Compare March 8, 2020 19:16
@AleksPetrakov
Copy link

AleksPetrakov commented Mar 18, 2020 •
edited
Loading

Hello @crisbeto !
Could you resolve any conflicts, please? Merging is blocked yet.
Thanks for improving this useful feature!

@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 5458aee to 1b98431 Compare March 20, 2020 22:21
@ghost ghost mentioned this pull request Apr 3, 2020
Closed
Splaktar
Splaktar approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM, this would be really useful to a lot of people who need to customize their paginators.

src/material/paginator/paginator.md
The current pageSize will always appear in the dropdown, even if it is not included in
pageSizeOptions.

If you want to customize some of the optional of the `mat-select` inside the `mat-paginator`, you
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you want to customize some of the optional of the `mat-select` inside the `mat-paginator`, you
If you want to customize some of the options of the `mat-select` inside the `mat-paginator`, you

@andrewseguin andrewseguin added lgtm action: merge The PR is ready for merge by the caretaker labels Jul 20, 2020
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@MrBacony
Copy link

Hi Guys is there a reason why this pr was not merged? Are there some concerns?

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 1b98431 to 9728cb3 Compare February 26, 2022 08:07
@crisbeto crisbeto changed the title feat(paginator): add input for configuring the underlying select feat(material/paginator): add input for configuring the underlying select Feb 26, 2022
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from 9728cb3 to bcc98fb Compare February 26, 2022 08:18
@crisbeto
feat(material/paginator): add input for configuring the underlying se…
64ff1c4 
…lect

Since we hide the underlying `MatSelect` inside the `MatPaginator`, the user doesn't have the ability to configure some of the inputs. These changes introduce an input that proxy some of the supported properties to the select.

Fixes angular#13646.
@crisbeto crisbeto force-pushed the 13646/paginator-select-config branch from bcc98fb to 64ff1c4 Compare February 26, 2022 08:24
@crisbeto crisbeto requested a review from a team as a code owner February 26, 2022 08:24
@crisbeto crisbeto merged commit 5fc655b into angular:master Feb 26, 2022
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 14, 2022
Closed
@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 Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@andrewseguin andrewseguin andrewseguin approved these changes

@Splaktar Splaktar Splaktar approved these changes

Assignees

@jelbourn jelbourn

@andrewseguin andrewseguin

Labels
action: merge The PR is ready for merge by the caretaker P4 A relatively minor issue that is not relevant to core functions target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature for mat-paginator; [disableOptionCentering]
8 participants
@crisbeto @jelbourn @AleksPetrakov @MrBacony @Splaktar @andrewseguin @googlebot @mmalerba