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.

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

Allow pt-disabled colors in label, select and input-group, and update documentation #448

Merged
merged 4 commits into from
Jan 12, 2017

Conversation

leebyp
Copy link
Contributor

@leebyp leebyp commented Jan 11, 2017

Fixes #402

Checklist

  • Update documentation

Changes proposed in this pull request:

  • Allow disabled label color with .pt-disabled.
  • Allow input-group icon color with .pt-disabled.
  • Allow select arrow color with .pt-disabled.
  • Update CSS API documentation for :disabled to include .pt-disabled usage.

@blueprint-bot
Copy link

Fix label and select colors when disabled, and update pt-disabled documentation

Preview: docs
Coverage: core | datetime

@giladgray
Copy link
Contributor

this is not really a "fix" since you're implementing a new modifier.

@giladgray
Copy link
Contributor

easy way to test: tick the "Inline" switch in Popover example.

image

const className = classNames(
Classes.CONTROL,
typeClassName,
this.props.className,
Copy link
Contributor

Choose a reason for hiding this comment

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

this.props.className should always come last in classNames() so user classes take precedence.

Classes.CONTROL,
typeClassName,
this.props.className,
{[Classes.DISABLED]: this.props.disabled});
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces inside braces. { [key]: val }

@@ -169,7 +174,7 @@ $control-checked-background-color-active: nth(map-get($button-intents, "primary"
</label>

:checked - Checked
:disabled - Disabled
:disabled - Disabled. This should be used with a <code>.pt-disabled</code> modifier (not shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't explain how to use it. say something about "add .pt-disabled to .pt-control element to change text color," and for all the similar notes below.

@@ -51,7 +51,7 @@ Markup:
<button class="pt-button pt-minimal pt-intent-primary pt-icon-arrow-right" {{:modifier}}></button>
</div>

:disabled - Disabled input. Must be added separately to the <code>&#60;input&#62;</code> and <code>&#60;button&#62;</code>.
:disabled - Disabled input. Must be added separately to the <code>&#60;input&#62;</code> and <code>&#60;button&#62;</code>. Should be used with a <code>.pt-disabled</code> modifier (not shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

"Also add .pt-disabled to .pt-input-group for icon coloring."

@@ -76,7 +76,7 @@ Markup:
<input class="pt-input" {{:modifier}} type="search" placeholder="Search input" dir="auto" />
</div>

:disabled - Disabled
:disabled - Disabled. This should be used with a <code>.pt-disabled</code> modifier (not shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be necessary. disabled attribute is sufficient for .pt-input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example includes an icon - reworded to be more obvious.

@@ -28,7 +28,7 @@ Markup:
</select>
</div>

:disabled - Disabled
:disabled - Disabled. This should be used with a <code>.pt-disabled</code> modifier (not shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

"Add .pt-disabled to .pt-select for icon coloring."

@leebyp leebyp changed the title Fix label and select colors when disabled, and update pt-disabled documentation Allow pt-disabled colors in label, select and input-group, and update documentation Jan 11, 2017
@blueprint-bot
Copy link

reword documentation and cleanup

Preview: docs
Coverage: core | datetime

@leebyp
Copy link
Contributor Author

leebyp commented Jan 11, 2017

updated PR title, PR description and documentation changes

Classes.CONTROL,
typeClassName,
{ [Classes.DISABLED]: this.props.disabled },
this.props.className);
Copy link
Contributor

Choose a reason for hiding this comment

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

close paren should be on its own line, dedented back to original.

function(
    args
);

@@ -343,7 +348,7 @@ $control-checked-background-color-active: nth(map-get($button-intents, "primary"
</label>

:checked - Selected
:disabled - Disabled
:disabled - Disabled. Add <code>.pt-disabled</code> to <code>.pt-control</code> to change text color (not shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

"Also add..." to indicate that some additional action is suggested?

@blueprint-bot
Copy link

more rewording and cleanup

Preview: docs
Coverage: core | datetime

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

add .pt-disabled modifier to Labels markup please

@leebyp
Copy link
Contributor Author

leebyp commented Jan 12, 2017

Updated the Disabled Labels part of the docs

@blueprint-bot
Copy link

update disabled labels docs

Preview: docs
Coverage: core | datetime

@giladgray giladgray merged commit 899f564 into master Jan 12, 2017
@giladgray giladgray deleted the bl/fix-labels-on-disabled-controls branch January 12, 2017 21:48
@giladgray giladgray mentioned this pull request Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants