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(expansion): Allow expansion indicator positioning #6917

Closed
wants to merge 2 commits into from
Closed

feat(expansion): Allow expansion indicator positioning #6917

wants to merge 2 commits into from

Conversation

josephperrott
Copy link
Member

#6726

Allows expansion indicator to be positioned at start, end and hidden.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 7, 2017
@josephperrott josephperrott force-pushed the expansion-panel branch 2 times, most recently from 8769ebf to 83f2690 Compare September 7, 2017 17:38
jelbourn
jelbourn reviewed Sep 7, 2017
View reviewed changes
src/lib/expansion/expansion-panel.ts Outdated
/** Whether the toggle indicator should be hidden. */
@Input() hideToggle: boolean = false;
/** The positioning of the expansion indicator. */
@Input() togglePosition: string = 'end';
Copy link
Member

Choose a reason for hiding this comment

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

The type would be 'start' | 'end' | 'hidden'

@crisbeto @kara what do you think about this API vs. having both togglePosition (only start/end) and hideToggle?

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 I prefer the single togglePosition since it avoids bloating the API for a somewhat minor feature. It's also consistent with other inputs like the MdFormField.floatPlaceholder which is 'always' | 'never' | 'auto'.

jelbourn
jelbourn reviewed Sep 7, 2017
View reviewed changes
src/lib/expansion/expansion-panel-header.scss Outdated
*/
.mat-expansion-indicator::after {
.mat-expansion-indicator-container {
margin-bottom: 2.83px;
Copy link
Member

Choose a reason for hiding this comment

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

These is weirdly specific- is there any way this can be calculated or set as the result of other known values?

src/lib/expansion/expansion-panel-header.ts Outdated
if (this.panel.togglePosition === 'start') {
return -1;
}
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

return this.panel.togglePosition === 'start' ? -1 : 1;

src/lib/expansion/expansion-panel.ts Outdated
}
return this.hideToggle;
return this.togglePosition;
Copy link
Member

Choose a reason for hiding this comment

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

return this.accordion ? this.accordion.togglePosition : this.togglePosition;

@josephperrott josephperrott force-pushed the expansion-panel branch 3 times, most recently from bd5e96f to b2b6c3f Compare September 12, 2017 15:45
@josephperrott josephperrott changed the title feat: Allow expansion indicator positioning feat(expansion): Allow expansion indicator positioning Sep 12, 2017
@josephperrott josephperrott added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful pr: needs review labels Sep 15, 2017
@josephperrott josephperrott force-pushed the expansion-panel branch 2 times, most recently from 20d7cd1 to 81361a7 Compare October 5, 2017 22:19
@josephperrott
Copy link
Member Author

@jelbourn When you have a moment can you take a look at this PR? I don't remember where we landed on this one.

@josephperrott josephperrott force-pushed the expansion-panel branch 2 times, most recently from 2d63a28 to ca027bf Compare October 12, 2017 15:26
tinayuangao and others added 2 commits October 16, 2017 08:55
@tinayuangao @josephperrott
docs(a11y): add accessibility sections to datepicker, menu, slide … (a…
6bc207a 
…ngular#6710)
@josephperrott
Allow placement of expansion panel indicator.
77cd36f 
BREAKING CHANGE: Expansion panel toggle indicicator is no longer hidden with a boolean input property hideToggle, but is not controlled with a
string input property togglePosition.  Available positions are: start, end and hidden.  Defaults to hidden.
@jelbourn jelbourn removed the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Oct 25, 2017
@jelbourn
Copy link
Member

@josephperrott as discussed we're going to keep hideToggle and make togglePosition separate to avoid the breaking API change.

@josephperrott
Copy link
Member Author

Closing in favor of #8199

@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 7, 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

@crisbeto crisbeto crisbeto left review comments

Assignees

@jelbourn jelbourn

Labels
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.
5 participants
@josephperrott @jelbourn @crisbeto @googlebot @tinayuangao