Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove getCellActions prop #1845

Merged
merged 3 commits into from
Dec 16, 2019
Merged

Remove getCellActions prop #1845

merged 3 commits into from
Dec 16, 2019

Conversation

amanmahajan7
Copy link
Contributor

A separate getCellActions prop is not required and cell actions feature can be easily implemented using a custom formatter.

Next we can remove props to implement subrows also

@amanmahajan7 amanmahajan7 self-assigned this Dec 15, 2019
@amanmahajan7 amanmahajan7 requested review from nstepien and qili26 and removed request for nstepien December 15, 2019 22:12
width: 200
width: 200,
cellClass: 'rdg-cell-action',
formatter({ row, value }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want we can use the new cellContentRenderer API here.

    cellContentRenderer({ rowData }) {
      const value = rowData.county;
      if (rowData.id === 'id_0') {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably do not need cellContentRenderer once subrows functionality is removed. We can also delete CellContent component and use formatter as cellContentRenderer. We can then replace defaultCellContentRenderer with defaultFormatter. Wdyt?

We can merge this PR after the lerna removal PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

IMO RDG shouldn't access the value from the row object, so we can have renderers like

({ rowData }) => rowData.nested.value;
({ rowData }) => rowData.x + rowData.y;

for example. And it might simplify types as well, maybe.

@qili26
Copy link
Contributor

qili26 commented Dec 16, 2019

Is this getCellActions() only used for customized html actions passed into RDG cell? I had never used this before, so want to double check.

@nstepien nstepien merged commit 47700b6 into pre-canary Dec 16, 2019
@nstepien nstepien deleted the am-kill-cell-actions branch December 16, 2019 20:05
@amanmahajan7
Copy link
Contributor Author

That is correct. It basically provides a way to add actions to a cell but the same functionality can be achieved with a custom formatter. Check the cell actions example. Also with this change we can use any component for the actions. .

@amanmahajan7 amanmahajan7 mentioned this pull request Jan 17, 2020
@ValYouW
Copy link

ValYouW commented Sep 5, 2020

@amanmahajan7 columns definition is usually done "outside" of the component, same goes for the formatter, which makes it difficult to do anything with an action fired from within the formatter - no way to "communicate" with component.
If the params to the formatter included some callback function that when called triggers another callback on the grid it would have been easier. Something like onRowChange that is passes to Editor2

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

Successfully merging this pull request may close these issues.

None yet

4 participants