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(placeholder): add global placeholder options #4681

Merged
merged 1 commit into from
Jun 8, 2017
Merged

feat(placeholder): add global placeholder options #4681

merged 1 commit into from
Jun 8, 2017

Conversation

RobJacobs
Copy link
Contributor

  • Implement global placeholder options so the
    default float option can be set to something
    other than 'auto'

Fixes #4311

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label May 19, 2017
@RobJacobs
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels May 19, 2017
@jelbourn jelbourn requested a review from mmalerba May 23, 2017 16:48
mmalerba
mmalerba reviewed May 25, 2017
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

Cool idea, I like it :)

src/lib/core/placeholder/placeholder-options.ts Outdated
import { OpaqueToken } from '@angular/core';

/** OpaqueToken that can be used to specify the global placeholder options. */
export const MD_PLACEHOLDER_GLOBAL_OPTIONS = new OpaqueToken('md-placeholder-global-options');
Copy link
Contributor

Choose a reason for hiding this comment

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

OpaqueToken is deprecated now, you can use InjectionToken instead

src/lib/core/placeholder/placeholder-options.ts Outdated
/** Type for the available floatPlaceholder values. */
export type FloatPlaceholderType = 'always' | 'never' | 'auto';

export interface PlaceholderGlobalOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just PlaceholderOptions is fine

src/lib/select/select.ts Outdated
import {
FloatPlaceholderType,
PlaceholderGlobalOptions,
MD_PLACEHOLDER_GLOBAL_OPTIONS }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

@RobJacobs
Copy link
Contributor Author

@mmalerba Thanks, the idea was inspired by the global ripple options. Revisions made based on your suggestions.

mmalerba
mmalerba reviewed May 26, 2017
View reviewed changes
src/lib/core/placeholder/placeholder-options.ts Outdated
@@ -0,0 +1,11 @@
import { InjectionToken } from '@angular/core';

/** OpaqueToken that can be used to specify the global placeholder options. */
Copy link
Contributor

Choose a reason for hiding this comment

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

comment still says OpaqueToken

src/lib/core/placeholder/placeholder-options.ts Outdated
import { InjectionToken } from '@angular/core';

/** OpaqueToken that can be used to specify the global placeholder options. */
export const MD_PLACEHOLDER_GLOBAL_OPTIONS = new InjectionToken('md-placeholder-global-options');
Copy link
Contributor

Choose a reason for hiding this comment

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

InjectionToken<PlaceholderOptions>

src/lib/input/input-container.ts Outdated
@Optional() private _parentFormGroup: FormGroupDirective) { }
@Optional() private _parentFormGroup: FormGroupDirective,
@Optional() @Inject(MD_PLACEHOLDER_GLOBAL_OPTIONS)
placeholderGlobalOptions: PlaceholderOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

make the variable placeholderOptions then this can all fit on one line

src/lib/select/select.ts Outdated
@Attribute('tabindex') tabIndex: string) {
@Attribute('tabindex') tabIndex: string,
@Optional() @Inject(MD_PLACEHOLDER_GLOBAL_OPTIONS)
placeholderGlobalOptions: PlaceholderOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same, change the var name and format like input constructor so it can fit on one line

@RobJacobs
Copy link
Contributor Author

@mmalerba Revisions made, thanks.

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 26, 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
@RobJacobs
feat(placeholder): add global placeholder options (#4681)
31459c2 
* Implement global placeholder options so the
default float option can be set to something
other than 'auto'

Fixes #4311
@RobJacobs
Copy link
Contributor Author

@andrewseguin Rebased, thanks...

@mmalerba mmalerba 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 d0d79fd into angular:master Jun 8, 2017
@RobJacobs RobJacobs deleted the placeholder-global-option branch June 12, 2017 13:20
@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

@mmalerba mmalerba mmalerba approved these changes

Assignees

@mmalerba mmalerba

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-input-container: Allow setting floatPlaceholder globally
5 participants
@RobJacobs @googlebot @andrewseguin @mmalerba @jelbourn