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

Missing actions in "shared with me" lists #2077

Closed
4 tasks
PVince81 opened this issue Sep 26, 2019 · 21 comments
Closed
4 tasks

Missing actions in "shared with me" lists #2077

PVince81 opened this issue Sep 26, 2019 · 21 comments
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Something isn't working

Comments

@PVince81
Copy link
Contributor

PVince81 commented Sep 26, 2019

In "Shared with me" and "Shared with others", assuming we want the same design like in OC 10:

  • clicking on a folder must redirect to the actual folder in the "All files" view
  • clicking on a file must trigger matching file action like opening an editor
  • a trash icon must be available to "unshare from self"
  • actions for accept/deny for pending shares (to verify)
@PVince81 PVince81 added Type:Bug Something isn't working Priority:p2-high Escalation, on top of current planning, release blocker labels Sep 26, 2019
@PVince81 PVince81 added this to the Milestone 1: Phoenix for users milestone Sep 26, 2019
@PVince81 PVince81 self-assigned this Oct 14, 2019
@PVince81
Copy link
Contributor Author

It looks like SharedFilesList is a whole separate view and simply copying the markup for actions is not enough. We need to somehow make SharedFilesList extend FilesList or use mixins to populate it with the missing methods (like isEnabled, $_actionInProgress, ...)

@LukasHirt thoughts ?

@PVince81
Copy link
Contributor Author

  • wrong icon displayed on top

@PVince81
Copy link
Contributor Author

image

@PVince81
Copy link
Contributor Author

image

@PVince81
Copy link
Contributor Author

We need to somehow make SharedFilesList extend FilesList

or we could create a new sub-view that contains only the table and then populate it from outside

@PVince81
Copy link
Contributor Author

sub-view is tricky as the column names are not the same

@PVince81
Copy link
Contributor Author

another idea: pack the file actions into a new reusable view ?

@PVince81
Copy link
Contributor Author

somehow I'm not happy about how much we'd need to duplicate between the file list views, or even other lists. so far we only have list of files with actions.

maybe this is a good candidate for introducing a more complex components for listing files in the design system.

for example, something like this:

<oc-files-table :filesList="filesList" :bulkActions="bulkActions" :actions="enabledActions" :activeCount="activeCount" :loading="loadingFolder">
   <oc-files-columns>
      <!-- because we specified bulk actions, the table will automatically include checkboxes in the header and every row -->
      <oc-files-column :label="t('Name')" field="name" />
      <oc-files-column :label="t('size')" :field="$_humanSize(item.size) />
       ...
   </oc-files-columns>
</oc-files-table>

The columns are only defined once and contain both the label and how the contents is rendered.

The main element has many properties as we need the file list data, the available actions, etc.
I'd also expect the table component to also render the "empty folder" image. This could be provided also as a property.

Not sure if we can pull this off but if we do we could reuse this for different file lists like "shared with you", etc. There is also the risk that this one will have too many properties...

Thoughts ?

@LukasHirt
Copy link
Collaborator

It looks like SharedFilesList is a whole separate view and simply copying the markup for actions is not enough. We need to somehow make SharedFilesList extend FilesList or use mixins to populate it with the missing methods (like isEnabled, $_actionInProgress, ...)

@LukasHirt thoughts ?

We should split files list into smaller components. Then we can have all those different lists in one component and just switch the smaller ones and some specific methods.

Alternative then would be the approach with ODS component as @PVince81 described in the comment above. With this I'm a little scared with bringing such a complex component into ODS. Also if we start bringing such components into ODS we would need to think soon how to optimise ODS to start using only components that are really used in the current view - AFAIK at the moment we don't do this which would make the bundle really big.

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 17, 2019

Alternatively we could also simply bring this one component not to ODS but just define it internally in Phoenix as a vue component.

@PVince81
Copy link
Contributor Author

or is there a way for inheritance ?

@LukasHirt
Copy link
Collaborator

Alternatively we could also simply bring this one component not to ODS but just define it internally in Phoenix as a vue component.

Would be also an option, yes 👍

@PVince81
Copy link
Contributor Author

After long discussion with @LukasHirt we decided to have a go at providing the "oc-files-table" components, but use a slot for the columns.

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 21, 2019

To think about, because currently Accept/Deny are separate action markup and not in the general file action list:

<oc-file-list-actions :actions="actions">
    <oc-file-list-action @click="$_acceptHandler($currentRow)">Accept</oc-file-list-action>
    <oc-file-list-action @click="$_declineHandler($currentRow)">Decline</oc-file-list-action>
</oc-file-list-actions>

how to $currentRow ??

@PVince81
Copy link
Contributor Author

  • also to consider: bulk actions and how to receive the implicit list of selected items ?

@PVince81
Copy link
Contributor Author

  • include virtual scrolling as part of the file list

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 21, 2019

Approaches discussed so far:

  • Approach 1: define a generic files table component (see Missing actions in "shared with me" lists #2077 (comment)
  • Approach 2: define components for most of the bits from the table
    • pros: suits well for file actions
    • cons: checkboxes are on another dimension and cannot be their own components
    • cons: does not cover most of the table parts that I'd like to encapsulate
  • Approach 3: base FileList.vue with extended components
    • cons: we should favor composition over inheritance as inheritance can become complex and require multiple levels to achieve what we want
    • cons: VueJS can only extend based on methods and properties, not templates

For now we'll go with approach 1 and see how far we can go, while fulfilling all requirements needed for the shared file lists, favorite list, trashbin.

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 21, 2019

it seems that with Vue JS we cannot create a component that would accept a structure like with #2077 (comment).

especially for parts where we want to have the checkbox column and actions column be implict, but let the component user define additional columns in the middle. it would look ugly:

<oc-files-table :filesList="filesList" :bulkActions="bulkActions" :actions="enabledActions" :activeCount="activeCount" :loading="loadingFolder">
   <oc-files-column-headers>
      <!-- needed for slots -->
      <template #columnHeaders>
         <oc-files-column-header v-translate>Name</oc-files-column-header>
         <oc-files-column-header v-translate>Size</oc-files-column-header>
      </template>
   </oc-files-column-headers>
   <oc-files-columns for="...">
      <!-- needed for slots -->
      <template #columns>
         <oc-files-column>...</oc-files-column>
         <oc-files-column>...</oc-files-column>
      </template>
   </oc-files-column>
</oc-files-table>

We'd need to define slots and to use them an additional "template" tag is needed.

Furthermode, for the "oc-files-columns" part the component user would need the for statement to iterate over file data. So the list component itself would not know that much about file data then.

@PVince81
Copy link
Contributor Author

Here is another approach where the column names and values are passed in as properties: https://vuejs.org/v2/examples/grid-component.html

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 21, 2019

I found this interesting example that integrates Google Maps and uses slots to implement map markers: https://css-tricks.com/using-scoped-slots-in-vue-js-to-abstract-functionality/

This is akin to how we thought about adding columns to the table with slots.

I don't like that the sub-elements need to take the main object in its properties as it's redundant.

@PVince81
Copy link
Contributor Author

  • have a look at scoped slots as it could hide the for loop inside the component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker Type:Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants