Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 435 - Table selected cell #444

Merged
alinastarkov merged 22 commits into
masterfrom
Issue435
May 31, 2019
Merged

Issue 435 - Table selected cell #444
alinastarkov merged 22 commits into
masterfrom
Issue435

Conversation

@alinastarkov

Copy link
Copy Markdown
Contributor

Fixes #435

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 28, 2019 16:29 Inactive

map.forEach((value, key) => console.log(value, key));
console.log(map.size);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔪 🕳️ 👍

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 29, 2019 18:34 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 30, 2019 14:03 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 30, 2019 18:23 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 30, 2019 18:25 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 30, 2019 18:48 Inactive

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few small comments here and there. Otherwise looks good.

Comment thread src/dash-table/derived/cell/wrapperStyles.ts Outdated
);

let isSelectedCell: boolean = selectedCells.some(cell => cell.row === index && cell.column_id === column.id);
if (isSelectedCell === true) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (isSelectedCell === true) {
if (isSelectedCell) {

The === true is redundant here.

Comment thread src/dash-table/derived/cell/wrapperStyles.ts
background-color: var(--selected-background);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can slice out that one ☝️

There's also an entry in Dropdown.css that can be removed (the border-radius is redundant anyway as the border: none is applied elsewhere in that file..

.dash-spreadsheet .cell--selected .Select-control {
    background-color: var(--selected-background);
    border-radius: 0;
}

id: 'table',
editable: true,
selected_cells: [[1, 1], [1, 2], [1, 3], [2, 1], [2, 2], [2, 3], [3, 1], [3, 2], [3, 3]],
active_cell: [1, 1],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hum.. one has to wonder why these still work 🤔

)
);

let isSelectedCell: boolean = selectedCells.some(cell => cell.row === index && cell.column === columnIndex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For consistency, use Ramda instead

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 31, 2019 14:28 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-444 May 31, 2019 16:03 Inactive

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💃

@alinastarkov
alinastarkov merged commit cbb0bd4 into master May 31, 2019
@alinastarkov
alinastarkov deleted the Issue435 branch June 6, 2019 18:09
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.

Table selected cell

3 participants