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(select): add md-optgroup component #4432

Merged
merged 3 commits into from
Jun 8, 2017
Merged

feat(select): add md-optgroup component #4432

merged 3 commits into from
Jun 8, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 8, 2017

Adds the md-optgroup component, which can be used to group options inside of md-select, in a similar way to the native optgroup.

Fixes #3182.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 8, 2017
jelbourn
jelbourn reviewed May 10, 2017
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.

Few comments from me, @kara should look at all of the positioning related stuff

src/lib/core/option/optgroup.ts
templateUrl: 'optgroup.html',
encapsulation: ViewEncapsulation.None,
inputs: ['disabled'],
host: {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should have role="group" as well

src/lib/select/select.spec.ts Outdated
@@ -735,7 +755,9 @@ describe('MdSelect', () => {
* Asserts that the given option is aligned with the trigger.
* @param index The index of the option.
*/
function checkTriggerAlignedWithOption(index: number): void {
function checkTriggerAlignedWithOption(index: number, selectInstance =
Copy link
Member

Choose a reason for hiding this comment

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

Need to update existing jsdoc

src/lib/select/select.ts Outdated
@@ -41,27 +41,30 @@ import 'rxjs/add/operator/startWith';
*/

/** The fixed height of every option element. */
export const SELECT_OPTION_HEIGHT = 48;
export const SELECT_ELEMENT_HEIGHT = 48;
Copy link
Member

Choose a reason for hiding this comment

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

SELECT_ITEM_HEIGHT (where item captures both groups and options)?

@jelbourn jelbourn requested a review from kara May 10, 2017 22:54
kara
kara approved these changes May 30, 2017
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits. Looks like this needs a rebase now too (probably my fault, sorry). Apply merge label when ready.

src/lib/core/option/optgroup.ts Outdated
export const _MdOptgroupMixinBase = mixinDisabled(MdOptgroupBase);

// Counter for unique group ids.
let nextId = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we make this name more specific to optgroup?

src/lib/select/select.ts

/** The max height of the select's overlay panel */
export const SELECT_PANEL_MAX_HEIGHT = 256;

/** The max number of options visible at once in the select panel. */
export const SELECT_MAX_OPTIONS_DISPLAYED = 5;
export const SELECT_MAX_OPTIONS_DISPLAYED =
Math.floor(SELECT_PANEL_MAX_HEIGHT / SELECT_ITEM_HEIGHT);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why this is now calculated from the other two constants. I'm nitpicking because now that Math.floor has been added as a top-level function call, the right side will be retained after dead code elimination even if the constant is unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to computing it, because it's easy to forget if we ever decide to change any of the other constants.

src/lib/select/select.ts Outdated
const selectedIndex = this._getOptionIndex(this._selectionModel.selected[0]);
let selectedIndex = this._getOptionIndex(this._selectionModel.selected[0]);

selectedIndex += this._getLabelCountBeforeOption(selectedIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since this isn't strictly the selectedIndex anymore, can we rename for clarity? selectedOffset or something?

@kara kara assigned crisbeto and unassigned kara and jelbourn May 30, 2017
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels May 31, 2017
@andrewseguin
Copy link
Contributor

Needs rebase

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 7, 2017
@crisbeto
feat(select): add md-optgroup component
951983c 
Adds the `md-optgroup` component, which can be used to group options inside of `md-select`, in a similar way to the native `optgroup`.

Fixes angular#3182.
@crisbeto
fix: address feedback
2ce96d3
@crisbeto
fix: address feedback
d139648
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 8, 2017
@andrewseguin andrewseguin merged commit d09aa89 into angular:master Jun 8, 2017
@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 Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@kara kara kara approved these changes

Assignees

@crisbeto crisbeto

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(select): supporting groups of options
5 participants
@crisbeto @andrewseguin @jelbourn @kara @googlebot