-
-
Notifications
You must be signed in to change notification settings - Fork 72
3.1 issue118 width behavior #130
Changes from all commits
2af1d3e
0646dc0
cdb93f5
50053fa
fcc1edb
70048e2
d5bed92
f03d02f
7bebb9d
4473467
966681f
75736ed
219da49
1bbc70e
172d3ab
ed7053c
1d93552
b91af6a
72231b8
61d3b74
443d380
47be714
4c16af0
c4a3609
54a9b49
617ce7b
caecaf5
e8b6751
f3b4671
bf28720
dd47d51
b3cee9f
a177af2
0c7be88
d85c35f
1544031
9334516
0af48b8
89fdd74
ed6c5c9
7c4ea41
4479b3f
23e0da7
453ad08
ec5f601
114c3b8
d0d4217
e62800f
0cd7e2c
7a0b167
fd2ec45
8033eb8
4935887
3afa253
b83bb86
995f1da
3de7cb4
b059da4
a7bd135
70ebd67
586babd
8f86751
ab03404
f84a0b4
00553a7
ab9b192
9c92a63
b4e521e
55efbe3
0b98396
6b2ba8b
529fcf2
ec6a6f2
da48b49
766cbc7
d064508
8e8429f
2283fc8
3f64aa1
c3cb872
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| export default class DOM { | ||
| public static getFirstParentOfType(element: HTMLElement, type: string): HTMLElement | undefined { | ||
| type = type.toUpperCase(); | ||
|
|
||
| let current: HTMLElement = element; | ||
| while (current) { | ||
| if (current.tagName.toUpperCase() === type) { | ||
| return current; | ||
| } | ||
|
|
||
| if (current.parentElement !== null) { | ||
| current = current.parentElement; | ||
| } else { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ import React, { | |
| } from 'react'; | ||
| import Dropdown from 'react-select'; | ||
|
|
||
| import DOM from 'core/browser/DOM'; | ||
|
|
||
| import { | ||
| ICellDefaultProps, | ||
| ICellProps, | ||
|
|
@@ -46,17 +48,23 @@ export default class CellInput extends PureComponent<ICellProps, ICellState> { | |
|
|
||
| return !dropdown ? | ||
| this.renderValue() : | ||
| (<Dropdown | ||
| ref='dropdown' | ||
| clearable={clearable} | ||
| onChange={(newValue: any) => { | ||
| onChange(newValue ? newValue.value : newValue); | ||
| }} | ||
| onOpen={this.handleOpenDropdown} | ||
| options={dropdown} | ||
| placeholder={''} | ||
| value={value} | ||
| />); | ||
| (<div className='dash-dropdown-cell-value-container dash-cell-value-container'> | ||
| {this.renderValue( | ||
| { className: 'dropdown-cell-value-shadow cell-value-shadow' }, | ||
| (dropdown.find(entry => entry.value === value) || { label: undefined }).label | ||
|
Contributor
Author
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. Use the label associated with the value, not the value itself for the shadow field
Contributor
Author
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. As for the input, the shadow div is used to size the table when using default column size |
||
| )} | ||
| <Dropdown | ||
| ref='dropdown' | ||
| clearable={clearable} | ||
| onChange={(newValue: any) => { | ||
| onChange(newValue ? newValue.value : newValue); | ||
| }} | ||
| onOpen={this.handleOpenDropdown} | ||
| options={dropdown} | ||
| placeholder={''} | ||
| value={value} | ||
| /> | ||
| </div>); | ||
| } | ||
|
|
||
| private renderInput() { | ||
|
|
@@ -80,22 +88,27 @@ export default class CellInput extends PureComponent<ICellProps, ICellState> { | |
| onDoubleClick: onDoubleClick | ||
| }; | ||
|
|
||
| return (!active && this.state.value === this.props.value) ? | ||
| const readonly = !active && this.state.value === this.props.value; | ||
|
|
||
| return readonly ? | ||
| this.renderValue(attributes) : | ||
| (<input | ||
| ref='textInput' | ||
| type='text' | ||
| value={this.state.value} | ||
| onBlur={this.propagateChange} | ||
| onChange={this.handleChange} | ||
| onKeyDown={this.handleKeyDown} | ||
| onPaste={onPaste} | ||
| {...attributes} | ||
| />); | ||
| (<div className='dash-input-cell-value-container dash-cell-value-container'> | ||
| {this.renderValue({ className: 'input-cell-value-shadow cell-value-shadow' })} | ||
|
Contributor
Author
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. shadow div vor column size
Contributor
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. Could you explain this shadow div stuff for me in more detail? Looks interesting, but not sure why it is needed.
Contributor
Author
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. When not applying a width to the columns, the columns are using the width of the content to decide what size they need to be. Since inputs force the width to be a lot bigger (inputs don't exactly respect normal width/sizing flow) than necessary (~150px) they create this undesirable effect of making columns bigger when a text|number cell is selected. That happens because text|number cells don't actually contain an input element when they are not selected, they just contain a div that shows the value (having thousands of inputs in a page causes significant slowdowns for all webkit based browsers, possibly others too) -- so.. since the input is alone in the table it's capable of forcing columns into a different size.. To prevent that, the input is made to be absolutely positioned and to fill the cell. Since the selected cell might be the one causing the column to have its current width, something has to force the column width when that input is present instead of the div.. hence the shadow div with the same style as the other normal div + some stuff to make it essentially invisible / have no impact on the render for the user.
Contributor
Author
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. I don't find this solution awesome to be honest but short of figuring out (1) another way for inputs to respect the width (or have decent perf with 1k+ inputs) and (2) force dropdowns to take the actual amount of space they are supposed to do (hint: there's absolute positioning the Select implementation, so that's not really for us to decide), I don't see a better way to do this for now. |
||
| <input | ||
| ref='textInput' | ||
| type='text' | ||
| value={this.state.value} | ||
| onBlur={this.propagateChange} | ||
| onChange={this.handleChange} | ||
| onKeyDown={this.handleKeyDown} | ||
| onPaste={onPaste} | ||
| {...attributes} | ||
| /> | ||
| </div>); | ||
| } | ||
|
|
||
| private renderValue(attributes = {}) { | ||
| const { value } = this.propsWithDefaults; | ||
| private renderValue(attributes = {}, value?: string) { | ||
| value = value || this.propsWithDefaults.value; | ||
|
|
||
| return (<div | ||
| {...attributes} | ||
|
|
@@ -175,7 +188,10 @@ export default class CellInput extends PureComponent<ICellProps, ICellState> { | |
|
|
||
| if (dropdown && document.activeElement !== dropdown) { | ||
| // Limitation. If React >= 16 --> Use React.createRef instead to pass parent ref to child | ||
| (dropdown.wrapper.parentElement as HTMLElement).focus(); | ||
| const tdParent = DOM.getFirstParentOfType(dropdown.wrapper, 'td'); | ||
|
Contributor
Author
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. cell structure changed a bit, we want the grandparent element, generalizing to finding first parent in hierarchy that matches expected type
Contributor
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. Maybe here you could try using refs too instead of manual DOM searching, they're not a React 16 specific feature, here's how they're used in 15 I believe: https://reactjs.org/docs/refs-and-the-dom.html#callback-refs
Contributor
Author
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. The wrapper is not accessible from the child (input) and the child is not accessible from the parent (injected through children) -- React 16 offers a new way to pass a parent ref into a child and that's what I had in mind for the future. I can see if I can do something here to make it better.
Contributor
Author
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. I don't think the ref approach is not good for practical reasons: We create the wrapper and clone it to include the child. At which point we would need to clone the child to insert the wrapper ref, which now points to the wrong child instance. Etc. |
||
| if (tdParent) { | ||
| tdParent.focus(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
To keep existing demo app behavior exactly as it was before