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

fix(datepicker): make touch UI work well on mobile #3853

Merged
merged 11 commits into from
Apr 14, 2017
Merged

fix(datepicker): make touch UI work well on mobile #3853

merged 11 commits into from
Apr 14, 2017

Conversation

mmalerba
Copy link
Contributor

depends on #3839

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 30, 2017
@mmalerba mmalerba requested review from kara and jelbourn April 14, 2017 16:51
jelbourn
jelbourn reviewed Apr 14, 2017
View reviewed changes
src/lib/datepicker/datepicker-content.scss Outdated
$md-datepicker-non-touch-calendar-width:
$md-datepicker-non-touch-calendar-cell-size * 7 + $md-datepicker-calendar-padding * 2;

// Note(mmalerba): Ideally the calendar would have a constant aspect ratio, no matter its size,
Copy link
Member

Choose a reason for hiding this comment

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

You can omit Note(mmalerba) since it's just an explanatory comment

src/lib/datepicker/datepicker.ts Outdated
@@ -49,6 +49,11 @@ let datepickerUid = 0;
selector: 'md-datepicker-content',
templateUrl: 'datepicker-content.html',
styleUrls: ['datepicker-content.css'],
host: {
'[class.mat-datepicker-content]': 'true',
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 just do

host: {
  'class': 'mat-datepicker-content'
}

And Angular will union it with the other classes and not overwrite (so long as it isn't a '[class'] binding)

src/lib/datepicker/datepicker.ts Outdated
host: {
'[class.mat-datepicker-content]': 'true',
'[class.mat-datepicker-content-touch]': 'datepicker.touchUi',
'[class.mat-datepicker-content-non-touch]': '!datepicker.touchUi',
Copy link
Member

Choose a reason for hiding this comment

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

Why have two classes instead of a style without the -touch class and then another with (where the latter would inherently be higher specificity)?

src/lib/dialog/dialog-container.ts Outdated
@@ -48,13 +48,25 @@ export type MdDialogContainerAnimationState = 'void' | 'enter' | 'exit' | 'exit-
])
],
host: {
'[class]': 'dialogConfigClasses',
Copy link
Member

Choose a reason for hiding this comment

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

This will overwrite all other classes on the dialog-container element

src/lib/dialog/dialog-config.ts Outdated
@@ -32,6 +32,9 @@ export class MdDialogConfig {
/** Position overrides. */
position?: DialogPosition;

/** Classes to add to the dialog container. */
containerClass?: string | string[] = '';
Copy link
Member

Choose a reason for hiding this comment

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

Do you need a class on the dialog container or on the root overlay element? I'd prefer to do the latter if we have to do this at all, as the dialog-container is meant to be a internal implementation detail

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'll undo this since we plan to move away from using dialog, I can hack around it for now

src/lib/dialog/dialog-container.ts Outdated
'[class.mat-dialog-container]': 'true',
'[attr.role]': 'dialogConfig?.role',
'[@slideDialog]': '_state',
'(@slideDialog.done)': '_onAnimationDone($event)',
},
})
export class MdDialogContainer extends BasePortalHost implements OnDestroy {
/** The CSS classes for the container specified in the dialog config. */
get dialogConfigClasses(): string {
Copy link
Member

Choose a reason for hiding this comment

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

This behavior would need unit tests if kept

@mmalerba
Copy link
Contributor Author

comments addressed

jelbourn
jelbourn approved these changes Apr 14, 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.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 14, 2017
@mmalerba mmalerba merged commit 43bbcde into angular:datepicker Apr 14, 2017
mmalerba added a commit that referenced this pull request Apr 14, 2017
@mmalerba
fix(datepicker): make touch UI work well on mobile (#3853)
57e1364 
* add keyboard support, break some tests

* add tests

* some finishing touches

* addressed some comments

* separate md-datepicker-content into own component

* fix(datepicker): make touch UI work well on mobile

* use vmin instead of media queries + vw, vh

* fix non-touch styles

* addressed comments

* fix lint error about vmin

* addressed comments
mmalerba added a commit that referenced this pull request Apr 20, 2017
@mmalerba
fix(datepicker): make touch UI work well on mobile (#3853)
b137740 
* add keyboard support, break some tests

* add tests

* some finishing touches

* addressed some comments

* separate md-datepicker-content into own component

* fix(datepicker): make touch UI work well on mobile

* use vmin instead of media queries + vw, vh

* fix non-touch styles

* addressed comments

* fix lint error about vmin

* addressed comments
mmalerba added a commit that referenced this pull request Apr 29, 2017
@mmalerba
fix(datepicker): make touch UI work well on mobile (#3853)
bcbb309 
* add keyboard support, break some tests

* add tests

* some finishing touches

* addressed some comments

* separate md-datepicker-content into own component

* fix(datepicker): make touch UI work well on mobile

* use vmin instead of media queries + vw, vh

* fix non-touch styles

* addressed comments

* fix lint error about vmin

* addressed comments
mmalerba added a commit that referenced this pull request May 5, 2017
@mmalerba
fix(datepicker): make touch UI work well on mobile (#3853)
1f2c39c 
* add keyboard support, break some tests

* add tests

* some finishing touches

* addressed some comments

* separate md-datepicker-content into own component

* fix(datepicker): make touch UI work well on mobile

* use vmin instead of media queries + vw, vh

* fix non-touch styles

* addressed comments

* fix lint error about vmin

* addressed comments
mmalerba added a commit that referenced this pull request May 9, 2017
@mmalerba
fix(datepicker): make touch UI work well on mobile (#3853)
1e9dfbd 
* add keyboard support, break some tests

* add tests

* some finishing touches

* addressed some comments

* separate md-datepicker-content into own component

* fix(datepicker): make touch UI work well on mobile

* use vmin instead of media queries + vw, vh

* fix non-touch styles

* addressed comments

* fix lint error about vmin

* addressed comments
@mmalerba mmalerba deleted the dp-mobile branch April 3, 2018 15:17
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@kara kara Awaiting requested review from kara

Assignees

@jelbourn jelbourn

@kara kara

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.
4 participants
@mmalerba @jelbourn @kara @googlebot