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
[Asset] Grid Improvements - Batch edit Asset metadata #5157
Conversation
…cts xlsx settings
…ct language match
…ctor grid column config
…column specific language config
# Conflicts: # bundles/AdminBundle/Resources/public/js/pimcore/object/helpers/gridTabAbstract.js
@dvesh3 Nice work! As the code is similar to one in DataObject controllers, do you have plans to extract the common base to separate services or helpers? @brusch as I've been looking into moving out some code from Controller is there a preffered place for code (sql, logic) out of them? |
Hi @kubaplas thanks for your review! we are trying our best to refactor the common functionality as it also leads to testing DataObject grid,..however it will be great if you can review it again after we merge this one and provide new PR for your changes :) For SQL queries, I would say preferred way is to go with Dao classes. |
# Conflicts: # bundles/AdminBundle/Controller/Admin/Asset/AssetHelperController.php # bundles/AdminBundle/Controller/Admin/AssetController.php # bundles/AdminBundle/Helper/GridHelperService.php # bundles/AdminBundle/Resources/public/js/pimcore/asset/helpers/gridConfigDialog.js # bundles/AdminBundle/Resources/public/js/pimcore/asset/helpers/gridTabAbstract.js # bundles/AdminBundle/Resources/public/js/pimcore/asset/listfolder.js # bundles/AdminBundle/Resources/public/js/pimcore/element/helpers/gridColumnConfig.js # bundles/AdminBundle/Resources/views/Admin/Index/index.html.php # bundles/CoreBundle/Resources/migrations/Version20191015131700.php # models/Asset/Service.php # models/GridConfig.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review, first round
Changes in this pull request
Resolves #1662
Additional info
This is dependent of #5093