proposal(dialog): better handling of scrollable content #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, the dialog's scrollable content (md-dialog-content) is limited to 65% of the viewport height, however on smaller screens the dialog still ends up being too high (see issue angular#2481). This proposal reworks the `md-dialog-content` to make it take up all of the remaining height, instead of being hardcoded to 65%. The max height is provided by the wrapper instead. The issue with this approach is that the direct parent of the `md-dialog-content` has to be `display: flex`, which can end up modifying the user's content. I see a couple of solutions, but I'm not sure which one to go with: 1. Conditionally add the flexbox styles, if the user has used `md-dialog-content`. That's what this PR is doing, however using `querySelector` to find out if the dialog content is there feels a little dirty. We're not able to use `@ViewChild` or `@ContentChild` to query for it. 2. Force the user to use our dialog content elements. This is what Material 1 was doing.
|
@jelbourn can you take a look at this? I wanted to pass it by here first before opening a PR on the main repo. |
crisbeto
pushed a commit
that referenced
this pull request
Aug 12, 2017
… demo) (angular#5562) * # This is a combination of 4 commits. # This is the 1st commit message: add lib files for sticky-header add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify fix some code according to PR review comments change some format to pass TSlint check add '_' before private elements delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable refine code encapsulate 'set style for element' change @input() Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' add const STICK_START_CLASS and STICK_END_CLASS Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; encapsulate reset css style operation for sticky header. delete unnecessary gloable variables sticky-header-rebased add one test && fixed bug 'sticky-header not working on iPhone', and bug 'reshape too obviously when being sticked'. && made some change according to the design doc('md-stick --> cdkSticky') make the sticky element being scrolled up smoothly when being unsticked Add code to support optional parentRegion input add add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix delete unused files encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix clean unused files delete sticky-header delete styicky-heade demo delete sticky-header lib revert public_api delete sticky-header e2e files delete.firebasesrc revert revert revert revert add css styles for selection-list add selection-list structures in list.ts initial version of selection-list all src/lib code fix tslint error fix tslint error applied '[class.mat-list-item-content-reverse]="checkboxPosition == 'after'"', deleted comments in code Deleted 'MdSelectionListCssMatStyler' class Added doc for 'md-selection-option' Added binding expression for 'attr.aria-selected' and 'attr.aria-disabled' Added a value and a disabled @input() property Changed 'isSelected' to 'selected'. And added '@input(selected)' for it.(Using 'coerceBooleanProperty') changed '_slist' to 'selectionList' and deleted 'MdSelectionListCheckboxer' class Deleted navList in selection-list-option changed 'onchange()' to toggle(). And added doc for it checkedItems -> selectedOptions deleted 'pCheckbox' and store 'this' instead of HTMLelement in 'optionlist' optimized toggle() with 'this._selected = !this._selected' Added 'Tab' moves focus on selection-list, 'UP_ARROW' and 'DOWN_ARROW' moves focus within selection-list. And use 'SPACE' to select checkbox Added another empty line at the end of each file remove comment line in CSS file Deleted 'webkit-justify-content' in CSS file Deleted 'mat-list-item-content-reverse' to these existing styles; the 'mat-list-item-content-reverse' class should only have the extra styles you need to reverse the display Deleted 'mat-list-item-content' and 'mat-list-item-content-reverse' and '<a>' styles in '.mat-selection-list' Deleted unused 'mat-checkbox' class add ".mat-list-option" class in css file added 'mat-list-option' class fix toggle() disabled Deleted empty class Deleted unused import Removed 'if (this.selectable)' check moved styles in '.mat-selection-list .mat-list-item' to '.mat-list-option'. Deleted position, box-sizing, and height in '.mat-list-item-content-reverse' in list.scss file put md-selection-list and md-list-option in separate files(selection-list.ts and list-option.ts) # This is the commit message #2: nit # This is the commit message #3: correct imports in index.ts # This is the commit message #4: use ' @ContentChildren(MdListOption) options;' instead of QueryList * add lib files for sticky-header add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify fix some code according to PR review comments change some format to pass TSlint check add '_' before private elements delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable refine code encapsulate 'set style for element' change @input() Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' add const STICK_START_CLASS and STICK_END_CLASS Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; encapsulate reset css style operation for sticky header. delete unnecessary gloable variables sticky-header-rebased add one test && fixed bug 'sticky-header not working on iPhone', and bug 'reshape too obviously when being sticked'. && made some change according to the design doc('md-stick --> cdkSticky') make the sticky element being scrolled up smoothly when being unsticked Add code to support optional parentRegion input add add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix delete unused files encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix clean unused files delete sticky-header delete styicky-heade demo delete sticky-header lib revert public_api delete sticky-header e2e files delete.firebasesrc revert revert revert revert add css styles for selection-list add selection-list structures in list.ts initial version of selection-list all src/lib code fix tslint error fix tslint error applied '[class.mat-list-item-content-reverse]="checkboxPosition == 'after'"', deleted comments in code Deleted 'MdSelectionListCssMatStyler' class Added doc for 'md-selection-option' Added binding expression for 'attr.aria-selected' and 'attr.aria-disabled' Added a value and a disabled @input() property Changed 'isSelected' to 'selected'. And added '@input(selected)' for it.(Using 'coerceBooleanProperty') changed '_slist' to 'selectionList' and deleted 'MdSelectionListCheckboxer' class Deleted navList in selection-list-option changed 'onchange()' to toggle(). And added doc for it checkedItems -> selectedOptions deleted 'pCheckbox' and store 'this' instead of HTMLelement in 'optionlist' optimized toggle() with 'this._selected = !this._selected' Added 'Tab' moves focus on selection-list, 'UP_ARROW' and 'DOWN_ARROW' moves focus within selection-list. And use 'SPACE' to select checkbox Added another empty line at the end of each file remove comment line in CSS file Deleted 'webkit-justify-content' in CSS file Deleted 'mat-list-item-content-reverse' to these existing styles; the 'mat-list-item-content-reverse' class should only have the extra styles you need to reverse the display Deleted 'mat-list-item-content' and 'mat-list-item-content-reverse' and '<a>' styles in '.mat-selection-list' Deleted unused 'mat-checkbox' class add ".mat-list-option" class in css file added 'mat-list-option' class fix toggle() disabled Deleted empty class Deleted unused import Removed 'if (this.selectable)' check moved styles in '.mat-selection-list .mat-list-item' to '.mat-list-option'. Deleted position, box-sizing, and height in '.mat-list-item-content-reverse' in list.scss file put md-selection-list and md-list-option in separate files(selection-list.ts and list-option.ts) nit correct imports in index.ts use ' @ContentChildren(MdListOption) options;' instead of QueryList change to check 'focusedIndex != null'. Set The tabindex of list option as -1 when the selection-list is disabled change ['class': 'mat-list-item, mat-list-option',] to ['class': 'mat-list-item mat-list-option',]; Don't want a comma between class names. fix typo in comments fix Binded 'aria-disabled' for selection-list.ts rename to '_optionsChangeSubscription' typo Instead of defining the disabled getter/setter yourself, you can include it via mixin. Removed ' selectable' property Deleted unnecessary 'this._isValidIndex(optionIndex)' check import switchMap Added SwitchMerge nit revert revert revert2 revert3 r4 r5 r2 r3 r4 r5 revert fix rxjs import Changed to use 'RxChain' instead of directly importing 'observer/add/' delete unusd import Added exports for selectionlist fix tslint check fix tslint check fix tslint check Add tests for selection-list and list-option fix typo in test Added new test on native element's focus & blur Added check expect(selectList.selected.length).toBe(0);before the toggle nit Added test on desecting an option fix tslink check fix typo in doc refine docs fix 'What happens if the selection list is disabled later? Would the list option become disabled?' Changed all the test name to the format of 'should ...' rename Deleted unused '_tabOutSubscription' return RxChain.from(this.options.changes)... fix tsLint check fix tslint fix tslint error test can not pass on travel CI fix travelCI check try to fix nativeElement focus test revert fix test fix travis CI error fixing travis fixed~! update fix fix fix fix2 fix added test check on 'aria-selected' Added test checkou on 'aria-disabled' reformat structure changed to 'if (!fixture.componentInstance.isIE) { ... }' Change 'platform.SAFARI || platform.EDGE || platform.FIREFOX' to '!platform.IS_TRIDENT' fix travis check Added aris-disabled test fix travis test put disabled style in theme.scss use background color for disabled items deselect --> deselected destory -> destoryed optimize changed to "if (!this.disabled) { ... }" restructured test cases change function to private _keydown protected -> private indent change 'select' to 'selectChange' Better to delete if (this.selectionList.disabled) {...} part Use this.disabled to consider the case when parent selectionList is disabled? Deleted emty lines It would be better not to mutate the option disabled state based on the parent state; this can be error prone, as it's easy to miss updating one when the other changes. It's better to rely on the disabled getter for the option to account for the parent state. * fix after rebase * fix * change 'grey, 1000' to 'black' * remove unused import * fix test * put selection-list.ts and list-option.ts into one file~ and fix the test * fix google3 * fix * Add imports * fix * Deleted unused import
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the dialog's scrollable content (md-dialog-content) is limited to 65% of the viewport height, however on smaller screens the dialog still ends up being too high (see issue angular#2481). This proposal reworks the
md-dialog-contentto make it take up all of the remaining height, instead of being hardcoded to 65%. The max height is provided by the wrapper instead.The issue with this approach is that the direct parent of the
md-dialog-contenthas to bedisplay: flex, which can end up modifying the user's content. I see a couple of solutions, but I'm not sure which one to go with:md-dialog-content. That's what this PR is doing, however usingquerySelectorto find out if the dialog content is there feels a little dirty. We're not able to use@ViewChildor@ContentChildto query for it.