Improve UX

Hello,

Patch does not apply anymore.

git apply improve_UX_3162831-2.patch
error: le patch a échoué : cmis.libraries.yml:6
error: cmis.libraries.yml : le patch ne s'applique pas
error: le patch a échoué : cmis.module:110
error: cmis.module : le patch ne s'applique pas
error: le patch a échoué : css/cmis_browser.css:16
error: css/cmis_browser.css : le patch ne s'applique pas
improve_UX_3162831-2.patch:620: new blank line at EOF.
+

Hello,

1. +++ b/cmis.libraries.yml
   @@ -6,6 +6,12 @@ cmis-field:
   +    'https://cdn.datatables.net/1.10.21/js/jquery.dataTables.min.js': { type: external }
   

   I agree that datatables can be a quick way to achieve having a better interactive table, but I think it would be better, if CMIS spec allows it, to use CMIS for that and not JS.

2. +++ b/css/cmis_browser.css
   @@ -16,3 +16,33 @@
   +
   

   Why a blank line here?

3. +++ b/js/cmis-datatables.js
   @@ -0,0 +1,17 @@
   +        $('#cmis-browser').DataTable( {
   

   If there are several browsers on the page (unlikely to happens I agree). This will not work.

   Instead of an ID, a class should be targeted. A best practice is to have the CSS class prefixed with js-, example: js-cmis-browser

   The use of the context variable should be done to avoid unneeded DOM search.

   I thing jquery once should be used because I had the following error in a JS alert popin:

   DataTables warning: table id=cmis-browser - Cannot reinitialise DataTable. For more information about this error, please see http://datatables.net/tn/3

4. +++ b/src/CmisElement.php
   @@ -337,4 +347,37 @@ class CmisElement {
   +      'image' => '',
   +      'application/pdf' => '',
   +      'application/msword' => '',
   +      'application/vnd.openxmlformats-officedocument.wordprocessingml.document' => '',
   +      'application/vnd.oasis.opendocument.text' => '',
   +      'application/vnd.ms-excel' => '',
   +      'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' => '',
   +      'application/vnd.oasis.opendocument.spreadsheet' => '',
   

   I agree that Fontello icons is a quick way to get this done. The problem I see is that it is not extensible, so each time someone has a ne file type to manage, it would have to be regenerated.

   Also, I went quickly in Fontello wiki, https://github.com/fontello/fontello/wiki/How-to-use-fontello-fonts-in-y..., And why having an hexadecimal mapping and not having a tag with the correct class directly?

5. +++ b/templates/cmis-browser-folder-item.html.twig
   @@ -1,3 +1,5 @@
   +
   

   Be cautious with unnecessary line added or removed.

In overall, thanks for the patch and the ideas. But it can't be merged currently due to review points but mainly due to the fact that such UI decisions have to be more thought due to the impact on websites.

You can still post updated patches if you want. But in case you need to update them. Maybe it would be better to put that in your project directly.

I attached a rebased patch.

Regards,

Hello,

Thanks for the patch.

I have tested it and no more JS error!

A small improvement, do not feel obliged to do it right now. Because as said above, we will need to discuss it more to have proper integration.

+++ b/js/cmis-datatables.js
@@ -0,0 +1,22 @@
+          $('.js-cmis-browser').DataTable();
...
+          $('.js-cmis-browser').DataTable( {

$('.js-cmis-browser', context)

Regards,

Thanks @liber_t for the feedback and pushing it forward in the PHP CMIS client library!

Regarding the response:

If you need to use pagination then you would be limited to whichever sorting functions are provided by the CMIS service you connected

So I guess we need to see if it will be possible to dynamically detect what is exposed by the CMIS endpoint then provide a browser depending on that.