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

Issue 315: Clearable columns #497

Merged
merged 14 commits into from
Jul 17, 2019
Merged

Issue 315: Clearable columns #497

merged 14 commits into from
Jul 17, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Jul 11, 2019

Closes #315.

  • Update CHANGELOG with information about the feature and ref to this PR/issue

  • New tests for the feature

  • Visual tests for the column actions

  • Documentation (clearable usage, action icon override through CSS & table css prop)

  • Adds the ability to clear column (columns when merged) content without deleting the column itself.

  • When the clear action is done, the associated filters are also cleared. The behavior of delete as been made the same as for clear with regard to the fitlers.

  • some code rework to share filter state between FilterFactory and HeaderFactory now that header actions tie in with filtering

Re-using the currently in review changes done by @alinastarkov to have a different behavior based on whether columns are merged or not (https://github.com/plotly/dash-table/blob/f69106e5a9639ab1f7831f4cdabb0860bd1579f0/src/dash-table/utils/actions.js)

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 11, 2019 20:26 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 16:32 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 17:43 Inactive
}

return newMap;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isolating the map update logic - can be called for a single column update (user edits filter) or through header clear and delete column actions (which may impact multiple columns at the same time)

).join(' && ');

setFilter(globalFilter, rawGlobalFilter, map);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same idea here, isolating the state update logic for both scenarios.

};
}
clearColumnsFilter(map, affectedColumns, 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.

Delete and clear apply the same logic, only the action changes

return typeof flag === 'boolean' ?
flag :
!!flag && flag[i];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamable, clearable, deletable all use the same logic to decide whether the action is available

};

const filterFactory = new FilterFactory(augmentedPropsFn);
const headerFactory = new HeaderFactory(augmentedPropsFn);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

headers now need access to map/setFilter too, generalize usage

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 18:32 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 19:10 Inactive
- update changelog
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 19:11 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 12, 2019 19:19 Inactive
@Marc-Andre-Rivet Marc-Andre-Rivet requested review from alexcjohnson and removed request for alexcjohnson July 12, 2019 19:20
@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review July 12, 2019 19:51
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 15, 2019 21:02 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 16, 2019 19:46 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 16, 2019 21:17 Inactive
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Code looks great. I'd just like to see a percy test including both deletable and clearable to know a) what the Ø looks like in the default font, and b) how the two symbols look next to each other.

@Marc-Andre-Rivet
Copy link
Contributor Author

@alexcjohnson Good idea, adding a few visual tests. Keeping in mind that we have #495 as a follow up.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 17, 2019 13:37 Inactive
@alexcjohnson
Copy link
Collaborator

Thanks for the visual tests!
I wonder if ø might look more “icon-like” than Ø
Also, a thought: what if we put ø and × into a css ::before or something, so users could override these (without us having to add another prop 😏)?

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented Jul 17, 2019

Action "icons" can now be overridden through css or the table's css prop

css = [
    { selector: '.column-header--clear::before', rule: `content: 'C';` },
    { selector: '.column-header--delete::before', rule: `content: 'D';` },
    { selector: '.column-header--edit::before', rule: `content: 'E';` }
]

::before has wide ranging support but did a sanity check against Edge 15 and IE11, for safety.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 17, 2019 14:55 Inactive
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Love it! 💃

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-497 July 17, 2019 17:25 Inactive
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.

Ability to clear the contents of a column
3 participants