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

Add a disabled state to <TagInput> #1819

Merged
merged 8 commits into from Nov 21, 2017
Merged

Add a disabled state to <TagInput> #1819

merged 8 commits into from Nov 21, 2017

Conversation

marvinsum
Copy link
Contributor

Added an extra property to the TagInput component. Setting the property to true makes it non-interactive in the UI. Updated the docs and tests.

screen shot 2017-11-17 at 8 36 32 pm

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @marvinsum! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@blueprint-bot
Copy link

Added test for disabling a TagInput component

Preview: documentation
Coverage: core | datetime

@@ -157,6 +164,9 @@ export class TagInput extends AbstractComponent<ITagInputProps, ITagInputState>
{
[CoreClasses.ACTIVE]: this.state.isInputFocused,
},
{
[CoreClasses.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.

Put this in the same object that was already in place:

{
    [CoreClasses.ACTIVE]: this.state.isInputFocused,
    [CoreClasses.DISABLED]: this.props.disabled,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmslewis ah yes, I didn't realize you could do that.

.find(".pt-input-ghost")
.first()
.prop("disabled"),
".pt-input-ghost does not have a 'disabled' attribute",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Use the same language here to clarify whether this is what the state should be vs what it is: .pt-input-ghost should have a 'disabled' attribute

@@ -23,6 +23,13 @@ import {
import * as Classes from "../../common/classes";

export interface ITagInputProps extends IProps {
/**
* Whether the component is non-interactive.
* Note that you'll also need to disable the component's rightElement, if appropriate.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap comments to 80-char lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap in back ticks: rightElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmslewis does the linter not enforce comments < 80-char lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, it just enforces a global max of 120 chars. To achieve 80-char lines in comments, I use the VS Code Rewrap plugin: https://marketplace.visualstudio.com/items?itemName=stkb.rewrap

@cmslewis
Copy link
Contributor

Can we go ahead and disable the rightElement in the example when you toggle the switch?

2017-11-17 18 18 24

llorca
llorca previously requested changes Nov 18, 2017
@@ -48,6 +48,10 @@ $ti-icon-padding-large: ($pt-input-height-large - $pt-icon-size-standard) / 2;
padding: 0 ($input-padding-horizontal - $ti-padding);
}

&[disabled] {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, please also add support for .pt-disabled 👌 it's part of our generic modifiers

@marvinsum
Copy link
Contributor Author

@cmslewis @llorca shouldn't we leave it to whoever who uses the TagInput to figure out if the TagInput.rightElement should be disabled? The rightElement is most likely a Button, but not always; it could be any JSX.Element.

@blueprint-bot
Copy link

Update TagInput test messages for consistency

Preview: documentation | landing | table
Coverage: core | datetime

@blueprint-bot
Copy link

Support .pt-disabled class on .pt-ghost-input

Preview: documentation | landing | table
Coverage: core | datetime

@cmslewis
Copy link
Contributor

@marvinsum Yes, but we control the example, so we should demonstrate how to disable the rightElement in that code (it's also less surprising that way).

@cmslewis
Copy link
Contributor

@marvinsum ^ sorry, I realize now that you were probably asking if your current approach was reasonable. I think it's fine as is, yes. Just want to make sure the example behaves as a walk-up user would expect.

@marvinsum
Copy link
Contributor Author

@cmslewis ah, got it. I updated the docs to explain that and disabled the button when the TagInput is disabled.

@blueprint-bot
Copy link

Updated docs to include disabled rightElement example for TagInput

Preview: documentation | landing | table
Coverage: core | datetime

cmslewis
cmslewis previously approved these changes Nov 21, 2017
@cmslewis cmslewis dismissed stale reviews from llorca and themself November 21, 2017 01:04

Comments addressed

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Thanks!

@cmslewis cmslewis merged commit 82449de into master Nov 21, 2017
@cmslewis cmslewis deleted the ms/disabled-multiselect branch November 21, 2017 02:35
@adidahiya adidahiya mentioned this pull request Dec 6, 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.

None yet

5 participants