Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Default Styles #196

Merged
merged 15 commits into from
Nov 1, 2018
Merged

Default Styles #196

merged 15 commits into from
Nov 1, 2018

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Nov 1, 2018

  • readonly cells cursor
  • filter fields placeholder & styling

image

image

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-196 November 1, 2018 17:52 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-196 November 1, 2018 17:53 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-196 November 1, 2018 17:53 Inactive
@@ -67,6 +69,7 @@ export default class IsolatedInput extends PureComponent<IProps, IState> {
type='text'
value={this.state.value || ''}
onChange={this.handleChange}
placeholder={placeholder}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding placeholder prop... wondering if we couldn't just compose off the dash-core-components input to get all the base prop types since the component works standalone. Might be an overkill, just thinking out loud here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's not a bad idea. How would that work though? Maybe if we were to redo core-components so that it's built more as a JS library, that both dash-renderer and other repo's like dash-table could use?

Copy link
Member

Choose a reason for hiding this comment

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

We can't really do that because components can't be properties right now in Dash. In the future, they could be. Otherwise, we'll need to copy over the properties one-by-one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is purely internal, I was thinking that just importing the component would work

@@ -10,7 +10,6 @@ interface IColumnFilterProps {
classes: string;
columnId: ColumnId;
isValid: boolean;
property: ColumnId;
setFilter: SetFilter;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

property was redundant with columnId and unused

input::placeholder {
color: transparent;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only display the placeholder if (1) the filter is the first (not selected by & + .dash-filter), otherwise only show if hovered or focused

.cell--uneditable input {
cursor: not-allowed;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the cursor style on readonly cells

columnStaticStyle: any,
data: Data
data: Data,
selectedCells: SelectedCells
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typing error here that went unnoticed because of the 'any'

@@ -36,7 +34,6 @@ function getter(
'dash-cell' +
` column-${columnIndex}` +
(active ? ' focused' : '') +
(!isEditable(editable, column.editable) ? ' cell--uneditable' : '') +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cell-uneditable is now useless -- removing it.. the cell can be customized through the style_cell prop, there's no need for this css selector

.add('with multiple columns', () => (<DataTable
{...props}
columns={['a', 'b', 'c'].map(id => ({ id: id, name: id.toUpperCase() }))}
/>));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two basic visual tests that make sure the first filter displays the placeholder and that the others do not.

Copy link
Contributor

@valentijnnieman valentijnnieman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -67,6 +69,7 @@ export default class IsolatedInput extends PureComponent<IProps, IState> {
type='text'
value={this.state.value || ''}
onChange={this.handleChange}
placeholder={placeholder}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's not a bad idea. How would that work though? Maybe if we were to redo core-components so that it's built more as a JS library, that both dash-renderer and other repo's like dash-table could use?

@chriddyp
Copy link
Member

chriddyp commented Nov 1, 2018

Sorry to be annoying, but can we go back to just monospace too? Seeing the other fonts in the docs makes our Monaco, ... it feel too opinionated right now.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

💃

@Marc-Andre-Rivet
Copy link
Contributor Author

@chriddyp Ok. Reverting the monospace changes and the corresponding changelog entry.

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

Successfully merging this pull request may close these issues.

None yet

3 participants