-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
bce9731
Add FileInput buttonText prop
thepocp 7c7c3b5
Change FileInput docs
thepocp 1fae376
Change FileInputExample
thepocp d3a5da0
Change attribute name
thepocp 8da9172
Add text overflow
thepocp 68597c3
Move paragraph
thepocp a37d946
Change style for docs
thepocp 37ef0af
Use new sheet
thepocp d02313b
Add file-upload-input-custom-text class
thepocp ad36948
Remove extra rule
thepocp 7a0aebc
Remove ref
thepocp c529229
Merge remote-tracking branch 'upstream/develop' into nm/file-input-text
thepocp 37a6870
Remove new line
thepocp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
62 changes: 62 additions & 0 deletions
62
packages/docs-app/src/examples/core-examples/fileInputExample.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 }); | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attr()
is supported for thecontent
property by all the browsers we care about: https://caniuse.com/#feat=css-gencontentnot 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
data-example-id
selector is wrongand I still think you don't need it overall
There was a problem hiding this comment.
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:
I moved rule in docs styles
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
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":
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:
... it is still a breaking change.
There was a problem hiding this comment.
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.