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

Refactor export handler from inline into own function #393

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

Corepex
Copy link
Contributor

@Corepex Corepex commented Jan 15, 2024

No description provided.

@Corepex Corepex added this to the 1.4.0 milestone Jan 15, 2024
@Corepex Corepex self-assigned this Jan 15, 2024
@Corepex Corepex marked this pull request as ready for review January 15, 2024 13:54
Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

LGTM
@kingjia90 wdyt? We would need it in 1.4

@Corepex Corepex requested a review from kingjia90 January 16, 2024 09:36
@kingjia90
Copy link
Contributor

kingjia90 commented Jan 16, 2024

Not sure how it works on ExtJS, but when setting an id, it would be "global" and unique, right?
Does it work properly when you have 2 or more tab with grids open?

My point is that we may need something like a suffix eg. _grid_2 that identifies the current tab/grid.

@Corepex
Copy link
Contributor Author

Corepex commented Jan 16, 2024

Not sure how it works on ExtJS, but when setting an id, it would be "global" and unique, right? Does it work properly when you have 2 or more tab with grids open?

My point is that we may need something like a suffix eg. _grid_2 that identifies the current tab/grid.

very good point - didn't think of that - i will check it ...

@kingjia90
Copy link
Contributor

kingjia90 commented Jan 16, 2024

By quickly look at the other PR, if we have to apply changes to admin ui classic bundle anyways, shouldn't be easier by refactoring to have this

pimcore.helpers.exportWarning(exportType, function (settings) {
this.exportPrepare(settings, exportType);
}.bind(this));

in an own method and call it (eg. currentGridTab.startExport('csv')) instead of simulating(?) a trigger/click of a button?

It then also overcome the "limit" of the copilot to be only exporting in CSV format. It could be easily extended to XLSX by passing a different exportType parameter.


image

Alternatively just could use getExportButtons() directly

@Corepex
Copy link
Contributor Author

Corepex commented Jan 16, 2024

@kingjia90, thanks for pointing me in the right direction. I moved the export functionality to a separate function (as you mentioned). With that it is now possible to start the export via the currentlyActiveTab ....
This solution feels much more reliable than the id (or class) guessing we had in mind.

Copy link
Contributor

@kingjia90 kingjia90 left a comment

Choose a reason for hiding this comment

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

LGTM, but need to change the PR's title before merging

@mattamon mattamon changed the title Added id to export button Refactor export handler from inline into own function Jan 16, 2024
@Corepex Corepex merged commit e93a0db into 1.x Jan 16, 2024
4 checks passed
@Corepex Corepex deleted the add_id_to_export_button branch January 16, 2024 13:25
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants