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

MINOR: specify export fields on Model itself #10388

Closed
wants to merge 1 commit into from

Conversation

sunnysideup
Copy link
Contributor

I would like to propose this change as it allows you to set export fields directtly on the model rather than relying on summary_fields or the gridfield display fields. I have never liked the way these two purposes were linked and this allows us to separate them.

In other words, you may want to export more / different fields than just the ones displayed in the summary list in the gridfield.

If you like it then I can also add the documentation to go with it.

Copy link
Member

@kinglozzer kinglozzer left a comment

Choose a reason for hiding this comment

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

I think this addition makes sense, one suggested change.

Does this also work for when ModelAdmin generates the export button? I seem to remember it has its own function that grabs export columns from summary fields

$singleton = DataObject::singleton($gridField->getModelClass());
if($singleton->hasMethod('getModelAdminExportFields')) {
return $singleton->getModelAdminExportFields();
}
Copy link
Member

@kinglozzer kinglozzer Jul 7, 2022

Choose a reason for hiding this comment

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

I think this block should come after if ($this->exportColumns) - if you manually specify fields against the GridField instance with ->setExportColumns() then that should take priority

@GuySartorelli
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@GuySartorelli
Copy link
Member

It seems like there's going to be no further activity on this pull request, so we’ve closed it for now. Please open a new pull-request if you want to re-approach this work, or for anything else you think could be fixed or improved.

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

3 participants