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(checkbox, slide-toggle): forward required attribute to input. #1137

Merged
merged 5 commits into from
Sep 23, 2016
Merged

fix(checkbox, slide-toggle): forward required attribute to input. #1137

merged 5 commits into from
Sep 23, 2016

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 30, 2016 •
edited
Loading

  • Now forwards the required attribute to the input.
  • This allows us to take advantage of the native browser behavior to prevent a form submission.
Slide Toggle Checkbox
image image

Fixes #1133,

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 30, 2016
@devversion devversion force-pushed the fix/slide-toggle-checkbox-required branch 2 times, most recently from 3474fed to 0a12419 Compare August 30, 2016 23:08
jelbourn
jelbourn reviewed Aug 31, 2016
View reviewed changes
src/lib/checkbox/checkbox.scss
// Move the input to the bottom and in the middle.
// Visual improvement to properly show browser popups when being required.
bottom: 0;
left: 50%;
Copy link
Member

@jelbourn jelbourn Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

transform: translateX(-50%)?

Also, it would be cool if we had a screenshot of what this looked like with and without this modification.

Copy link
Member Author

@devversion devversion Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I was actually going to upload some screenshots, but this Safari thing distracted me too much 😄

Screenshots are now in the PR description.

Also we already use an absolute position here, and I want the 50% to be relative to the inner container.

@jelbourn
Copy link
Member

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 31, 2016
@kara kara added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 31, 2016
@devversion devversion force-pushed the fix/slide-toggle-checkbox-required branch from 823307e to 5f07953 Compare September 6, 2016 21:03
@hansl hansl added in progress This issue is currently in progress and removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 7, 2016
hansl
hansl reviewed Sep 7, 2016
View reviewed changes
src/lib/checkbox/checkbox.ts
@@ -12,6 +12,7 @@ import {
ModuleWithProviders,
} from '@angular/core';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';
import {BooleanFieldValue} from '@angular2-material/core/annotations/field-value';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use @angular2-material/core. We don't support deep imports.

@hansl
Copy link
Contributor

hansl commented Sep 7, 2016

There was a bug uncovered by the build. Need to fix.

@hansl
Copy link
Contributor

hansl commented Sep 8, 2016

LGTM.

@hansl hansl added pr: lgtm and removed blocked This issue is blocked by some external factor, such as a prerequisite PR in progress This issue is currently in progress labels Sep 8, 2016
@devversion devversion force-pushed the fix/slide-toggle-checkbox-required branch from f7d9be6 to 21c92ce Compare September 9, 2016 21:21
@devversion
Copy link
Member Author

@jelbourn Rebased.

@devversion devversion force-pushed the fix/slide-toggle-checkbox-required branch 3 times, most recently from 5f1b84a to 6c788e8 Compare September 21, 2016 17:37
@devversion
fix(checkbox, slide-toggle): forward required attribute to input.
ae6acdc 
* Now forwards the required attribute to the input.
* This allows us to take advantage of the native browser behavior to prevent a form submission.

Fixes angular#1133,
@devversion
Fix linters
77a5d92
@devversion
Fix Browserstack test for super old Safari browser.
2cc90ec
@devversion
Safari 8 does not report input validity in forms, so the tests fail f…
0e430ea 
…or it..
@devversion devversion force-pushed the fix/slide-toggle-checkbox-required branch from 6c788e8 to a8b724a Compare September 22, 2016 15:30
@devversion
Copy link
Member Author

Rebased on top of the latest theme changes.

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 23, 2016
@kara kara merged commit 0e9383a into angular:master Sep 23, 2016
@lghinet
Copy link

lghinet commented Nov 4, 2016

hello,
when will this fix be published ?

@devversion devversion deleted the fix/slide-toggle-checkbox-required branch November 4, 2016 16:28
@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
No reviews
Assignees
No one assigned
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.

MD-Checkbox required-Attribute not passed to the underlying element
6 participants
@devversion @jelbourn @hansl @lghinet @kara @googlebot