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 FileInput buttonText prop #3560

Merged
merged 13 commits into from
Jul 8, 2019
9 changes: 8 additions & 1 deletion packages/core/src/components/forms/_file-input.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

@import "../../common/variables";
@import "../button/common";
@import "../../common/mixins";

/*
File input
Expand Down Expand Up @@ -94,6 +95,7 @@ $file-input-button-padding-large: ($pt-input-height-large - $pt-button-height) /
&::after {
@include pt-button();
@include pt-button-height($pt-button-height-small);
@include overflow-ellipsis();
position: absolute;
top: 0;
right: 0;
Expand All @@ -102,7 +104,7 @@ $file-input-button-padding-large: ($pt-input-height-large - $pt-button-height) /
width: $file-input-button-width;
text-align: center;
line-height: $pt-button-height-small;
content: "Browse";
content: attr(#{$ns}-button-text);
}

&:hover::after {
Expand Down Expand Up @@ -143,6 +145,11 @@ $file-input-button-padding-large: ($pt-input-height-large - $pt-button-height) /
}
}

#blueprint-documentation [data-example-id="file-input"] .#{$ns}-file-upload-input::after {
content: "Browse";
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this? shouldn't it be set as the default in the JS component?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think this block is needed, it doesn't seem to do anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to this block. Attr function has the fallback parameter, but it's not supported by browsers. How I can set style only for docs?
Screenshot 2019-06-18 at 23 45 07

Copy link
Contributor

Choose a reason for hiding this comment

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

attr() is supported for the content property by all the browsers we care about: https://caniuse.com/#feat=css-gencontent

not sure what you're trying to achieve here? this kind of selector would be the right way to do docs-only styles, but there are a couple issues

  • the styles should go in docs-app along with the other overrides; not in blueprint core
  • the data-example-id selector is wrong

and I still think you don't need it overall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant the second argument of the function attr (fallback)
If you don't use this style, the text is not displayed:
Screenshot 2019-06-19 at 00 13 30
I moved rule in docs styles

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, that's not good then... we don't want all users to have to add a style override just to get things working out of the box... and unfortunately I don't want to introduce a breaking change to the CSS API where it's required to supply a bp3-button-text attribute in order to get the text to render...

can we try using multiple CSS statements to achieve the fallback? if not, we might have to delay merging this PR as it could be a breaking change...

content: "Browse";
content: attr(#{$ns}-button-text);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I wrote something wrong. Sorry for my English.
This rule is only needed for the CSS block in the documentation. In other places it isn't needed and everything works well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you write like this, the second rule will overwrite the first. And there will be an empty string

content: "Browse";
content: attr (#{$ns}-button-text);

Copy link
Contributor

Choose a reason for hiding this comment

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

Your English is fine, no need to apologize :)

This rule is only needed for the CSS block in the documentation.

Unfortunately, this is not true for this library because the CSS-only styling API is part of Blueprint's public API (in addition to the JS components).

Some users render this markup to get a working file input with the word "Browse":

<label class="bp3-file-input">
  <input type="file"/>
  <span class="bp3-file-upload-input">Choose file...</span>
</label>

We don't want to remove support for this, otherwise it would be a regression... even though I understand that it's a minor syntax migration to change the API to this instead:

<label class="bp3-file-input">
  <input type="file"/>
  <span class="bp3-file-upload-input" bp3-button-text="Browse">Choose file...</span>
</label>

... it is still a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I solved the problem. The CSS is just the string content: "Borwse". When creating a component, a new style is created.
But for the documentation I had to leave the style, because the new style is applied.

}


// hack to force the button shadow to display since
// it doesn't render correct via the `pt-button` mixin
// stylelint-disable-next-line
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/components/forms/file-input.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
Use the standard `input type="file"` along with a `span` with class `@ns-file-upload-input`.
Wrap that all in a `label` with class `@ns-file-input`.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please move this paragraph to the CSS section? right around L26



@reactExample FileInputExample

@## Props

This component is a lightweight wrapper around the underlying `<label>` element
Expand Down
37 changes: 36 additions & 1 deletion packages/core/src/components/forms/fileInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ export interface IFileInputProps extends React.LabelHTMLAttributes<HTMLLabelElem
* @default "Choose file..."
*/
text?: React.ReactNode;

/**
* The button text.
* @default "Browse"
*/
buttonText?: string;
}

// TODO: write tests (ignoring for now to get a build passing quickly)
Expand All @@ -76,13 +82,27 @@ export class FileInput extends React.PureComponent<IFileInputProps, {}> {
public static displayName = `${DISPLAYNAME_PREFIX}.FileInput`;

public static defaultProps: IFileInputProps = {
buttonText: "Browse",
hasSelection: false,
inputProps: {},
text: "Choose file...",
};

private input: HTMLSpanElement | null = null;

public componentDidMount() {
this.changeButtonText(this.props.buttonText);
}

public componentWillReceiveProps(newProps: IFileInputProps) {
if (this.props.buttonText !== newProps.buttonText) {
this.changeButtonText(newProps.buttonText);
}
}

public render() {
const {
buttonText,
className,
disabled,
fill,
Expand All @@ -108,11 +128,26 @@ export class FileInput extends React.PureComponent<IFileInputProps, {}> {
return (
<label {...htmlProps} className={rootClasses}>
<input {...inputProps} onChange={this.handleInputChange} type="file" disabled={disabled} />
<span className={Classes.FILE_UPLOAD_INPUT}>{text}</span>
<span ref={this.createRef} className={Classes.FILE_UPLOAD_INPUT}>
{text}
</span>
</label>
);
}

private createRef = (input: HTMLSpanElement | null) => {
if (input !== null) {
this.input = input;
}
};

private changeButtonText = (text: string) => {
if (this.input) {
const NS = Classes.getClassNamespace();
this.input.setAttribute(`${NS}-button-text`, text);
}
};

private handleInputChange = (e: React.FormEvent<HTMLInputElement>) => {
Utils.safeInvoke(this.props.onInputChange, e);
Utils.safeInvoke(this.props.inputProps.onChange, e);
Expand Down
62 changes: 62 additions & 0 deletions packages/docs-app/src/examples/core-examples/fileInputExample.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright 2019 Palantir Technologies, Inc. All rights reserved.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as React from "react";

import { FileInput, FormGroup, H5, InputGroup } from "@blueprintjs/core";
import { Example, IExampleProps } from "@blueprintjs/docs-theme";

interface IFileInputExampleState {
buttonText?: string;
text?: string;
}

export class FileInputExample extends React.PureComponent<IExampleProps, IFileInputExampleState> {
public state: IFileInputExampleState = {};

public render() {
const { text, buttonText } = this.state;

return (
<Example options={this.renderOptions()} {...this.props}>
<FileInput text={text} buttonText={buttonText} />
</Example>
);
}

private renderOptions = () => {
const { text, buttonText } = this.state;

return (
<>
<H5>Props</H5>
<FormGroup label="Text">
<InputGroup placeholder="Choose file..." onChange={this.handleTextChange} value={text} />
</FormGroup>
<FormGroup label="Button text">
<InputGroup placeholder="Browse" onChange={this.handleButtonTextChange} value={buttonText} />
</FormGroup>
</>
);
};

private handleTextChange = (e: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ text: e.target.value });
};

private handleButtonTextChange = (e: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ buttonText: e.target.value });
};
}
1 change: 1 addition & 0 deletions packages/docs-app/src/examples/core-examples/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export * from "./dividerExample";
export * from "./drawerExample";
export * from "./dropdownMenuExample";
export * from "./editableTextExample";
export * from "./fileInputExample";
export * from "./focusExample";
export * from "./formGroupExample";
export * from "./hotkeyPiano";
Expand Down