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

[core] Add space in between Label 'text' and 'helperText'. #2180

Merged
merged 2 commits into from
Feb 26, 2018
Merged

[core] Add space in between Label 'text' and 'helperText'. #2180

merged 2 commits into from
Feb 26, 2018

Conversation

danielbh
Copy link
Contributor

Fixes #2175

Checklist

Changes proposed in this pull request:

Adds a space in between text and helperText for the Label component

Reviewers should focus on:

The updated Label component and new tests

Screenshot

Before

screen shot 2018-02-23 at 10 05 46 pm

After

screen shot 2018-02-23 at 11 15 07 pm

@danielbh
Copy link
Contributor Author

danielbh commented Feb 24, 2018

Good morning. Having a bit if trouble getting Circle to run on my new branch. Is this something you do on your end? Excited to start helping out.

@adidahiya
Copy link
Contributor

Looks like your build workflow did run, but it was canceled. Can you try to retrigger it? https://circleci.com/gh/danielbh/workflows/blueprint/tree/dh%2Fadd-space-between-text-and-helperText

@danielbh
Copy link
Contributor Author

danielbh commented Feb 25, 2018

Ok I re-triggered it. I had the "only build pull requests" option selected for some reason. I also needed to add a new commit.

@danielbh
Copy link
Contributor Author

Update labelTests.tsx

Preview: documentation | landing | table

@adidahiya
Copy link
Contributor

thanks!

@adidahiya adidahiya merged commit 3b44d24 into palantir:develop Feb 26, 2018
@@ -42,7 +42,7 @@ export class Label extends React.PureComponent<ILabelProps, {}> {
return (
<label {...htmlProps} className={rootClasses}>
{text}
<span className={classNames(Classes.TEXT_MUTED)}>{helperText}</span>
<span className={classNames(Classes.TEXT_MUTED)}>{helperText ? " " + helperText : ""}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

As the helper text is conditional, would it not be best to keep the span out of the DOM completely if helperText doesn't exist? e.g.

{helperText && <span className={classNames(Classes.TEXT_MUTED)}>{" " + helperText}</span>}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out Will. It seems cleaner to me. I’ll look if this affects the CSS. I can re-submit a PR tonight.

Copy link
Contributor

@will-stone will-stone Mar 20, 2018

Choose a reason for hiding this comment

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

Ah, the changes have broken the ability to add a React node as the helpertext e.g.

<Label
  text="Username"
  helperText={
      <span style={{ display: 'block', fontSize: '80%' }}>
          blah
       </span>
  }
>
  ...
</Label>

This now displays as : Username [object Object]

I suggest changing to:

{helperText && <span className={classNames(Classes.TEXT_MUTED)}>{" "}{helperText}</span>}

I'll log this as another issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[label] missing space between text and helperText
3 participants