-
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
[table] feat(EditableCell2): new placeholder prop #5421
Conversation
Now placeholders can be used in editable cell2 objects
yarn.lock
Outdated
@@ -5255,7 +5255,7 @@ first-mate@^7.0.2: | |||
event-kit "^2.2.0" | |||
fs-plus "^3.0.0" | |||
grim "^2.0.1" | |||
oniguruma "7.2.1" | |||
oniguruma "7.2.3" |
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.
This was fixed in #5407. Just merge in develop
Hey @ayxliu19, please revert your changes to |
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 think your intuition is right, that the editableTextProps={ placeholder }
API feels a bit weird, especially since you've implemented it in a way that the placeholder is rendered even when <EditableText>
is not present. It would be better to add a placeholder
prop to <EditableCell2>
, and mention that this new prop overrides editableTextProps.placeholder
in its documentation.
packages/table/src/cell/_cell.scss
Outdated
@@ -64,3 +64,7 @@ | |||
right: 0; | |||
top: 0; | |||
} | |||
|
|||
.#{$ns}-table-cell-text-placeholder { | |||
color: $input-placeholder-color; |
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.
we'll need to add dark theme styles as well, using $pt-dark-text-color-muted
@@ -33,6 +33,7 @@ Color aliases | |||
|
|||
$table-background-color: $light-gray5 !default; | |||
$dark-table-background-color: $dark-gray4 !default; | |||
$input-placeholder-color: $pt-text-color-muted !default; |
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 don't think you need this variable, especially since it's the same name as another (non-public) variable in core/src/components/forms. You can just reference the $pt-text-color-muted
and $pt-dark-text-color-muted
variables directly where you need the colors.
Hi @adidahiya . Thank you for the feedback. I have made the changes and recompiled the documentation, but it seems like currently EditableCell2Props are not being displayed in the Blueprint documentation page: https://blueprintjs.com/docs/#table/api.editablecell Would you like me to incorporate the same changes to EditableCell1 so that the changes are reflected in the documentation? |
Added dark theme css style for placeholder
Co-authored-by: Michael Wu <michael-yx-wu@users.noreply.github.com>
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.
@ayxliu19 good point about EditableCell2 not being shown in the docs -- I will fix that in a separate PR. No need to block on that for now, though.
}); | ||
|
||
cellContents = <div className={textClasses}>{savedValue}</div>; | ||
cellContents = <div className={textClasses}>{hasValue ? savedValue : this.props.placeholder}</div>; |
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.
This is good but I think we need to bring back the change to cellContents
a few lines above here... <EditableText placeholder={this.props.placeholder}>
should be set on L168, as suggested in the documentation of the new prop you just added.
I'm adding EditableCell2Props to the docs in this PR: #5449 |
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.
nice, thanks @ayxliu19 👍🏽
Fixes #5083
Changes proposed in this pull request:
Added a placeholder feature that can be inputted to the editableTextProps field in EditableCell2. The placeholder shows the placeholder string if the cell is blank.
Another Design:
Since this approach uses the editableTextProps field, it is awkward calling it outside of the editable text field that is rendered when isEditing is true in
blueprint/packages/table/src/cell/editableCell2.tsx
Line 148 in bf4c8be
A better approach may be to create a new placeholder field in the EditableCell2 class and use that instead of calling the one that is in editableTextProps.
Screenshot
Example: