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 intomasterfrom
Issue435
May 31, 2019
Merged

Issue 435 - Table selected cell #444
alinastarkov merged 22 commits intomasterfrom
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
Copy link
Copy Markdown
Contributor

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

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.

);

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.

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
Copy link
Copy Markdown
Contributor

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

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