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

Fix/quick view #1611

Merged
merged 39 commits into from Jul 19, 2019
Merged

Fix/quick view #1611

merged 39 commits into from Jul 19, 2019

Conversation

Crash--
Copy link
Contributor

@Crash-- Crash-- commented Apr 18, 2019

Several points in this PR :

  • We now have a concept of "view". View is based on our navigation system. Currently we have 4 views : folder / trash / recent / sharings

  • For each of this view we have a special "slice" of the redux store to store the concerned files :
    files : { shared: [], recent: [], trashed: [], folder: [] }

  • We have a selector to get the currentView based the latest dispatched action. It is not the displayedView but the desired one

  • Tried to use the redux store instead of the location when I could without rewriting a few big part of the app.

  • Added lot of documentation about internal stuff

@Crash--
Copy link
Contributor Author

Crash-- commented Apr 18, 2019

Copy link
Contributor

@y-lohse y-lohse left a comment

Choose a reason for hiding this comment

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

Nice, two neat tricks.

However, would it be hard to add tests for this? If not I think they would be really helpful here.

src/drive/web/modules/navigation/duck/reducer.js Outdated Show resolved Hide resolved
@Crash--
Copy link
Contributor Author

Crash-- commented Apr 18, 2019

I think adding tests to SharingsContainer should be doable since some are already there. For the "actions" part, it seems like a huge effort.

@y-lohse be prepared, tomorrow I'll have to work on a new task so... :D

@cozy-bot
Copy link

Visual Review - Please review screenshots, then restart build.

  • ❌ drive : ClassicationScenario 3- Trash and Restore : https://visualreview.cozycloud.cc/#/7/50/1020
  • ❌ drive : PublicViewerFeature 3- Cleanup Data : https://visualreview.cozycloud.cc/#/7/55/1025
  • @cedricmessiant
    Copy link
    Contributor

    I don't think you can be sure that another action has not been be dispatched in the meantime. If this action has nothing to do with files fetching you still want to update the store with recent files.

    files state may contain recent files, shared files, etc (and fetch the files "on demand") but the route should select only the files it is interested in.

    For example, the store could look like this:

    files: {
      shared: [],
      recent: [],
    },
    currentDisplayedFolder: 'recent'
    

    Of course it is a huge change and we probably can't do this to fix this bug but maybe we could create a technical card.

    @Crash--
    Copy link
    Contributor Author

    Crash-- commented Apr 19, 2019

    I don't think you can be sure that another action has not been be dispatched in the meantime. If this action has nothing to do with files fetching you still want to update the store with recent files.

    You right. After 2 minutes of thinking, we can receive UPLOAD_SUCCESS or other related sync stuff.

    At first, I wanted to do something similar at what you proposed. It was a bit more complicated although. Your solution should be doable.

    @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ drive : ClassicationScenario 2- Classification actions on file : https://visualreview.cozycloud.cc/#/7/49/1145
  • ❌ drive : ClassicationScenario 3- Trash and Restore : https://visualreview.cozycloud.cc/#/7/50/1147
  • ❌ drive : PublicViewerFeature 1- Prepare Data : https://visualreview.cozycloud.cc/#/7/53/1149
  • @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ photos : PhotosCrud 1- Photos Navigation : https://visualreview.cozycloud.cc/#/8/64/1146
  • @Crash-- Crash-- changed the title Fix/quick view [WIP] Fix/quick view Apr 23, 2019
    @Crash--
    Copy link
    Contributor Author

    Crash-- commented Apr 23, 2019

    Some news:

    We now have:

     files : { shared: [], recent: [], trashed: [], folder: [] },
    currentView: 'recent' // https://github.com/cozy/cozy-drive/pull/1611/commits/4eeea76b527218031fafffc93b997c8a7b56a8d8#diff-47554ec30330a7ef6eb836864a43b9f4R78

    Based on the currentView we return the right reducer path: 4eeea76#diff-47554ec30330a7ef6eb836864a43b9f4R425

    I found out some "issues" doing this stuff:

    Quand dans un nfolder, on supprimer des fichiers : 
    - TRash_files(ids)
     + delete_file(id) (x X)
    
    
    Quand on supprime depuis la corbeille des fichiers définitivement : 
    - Destroy_files(ids)
        + delete_file(id) (x X)
    

    But the delete_file is not dispatched by the click, but by the real time (4eeea76#diff-9d445e7faef1808a83ef82ea6ba523efR99). Since we're removing files here, we can have a race between this and this line

    ids: files.map(f => f.id),
    . Sometimes, the map doesn't have the right value.

    There is still a lot of work to be done:

    • I think I broke the pagination
    • We can refactor the Container stuff. Since we now have a specific path for each view, we can optimise them to ne re-render if they are not concerned.
    • We can remove lot of stuff (no need to store the latest action).
    • We can change some check. Currently we are checking if the displayFolder is null to know if we're displaying trash or not.

    @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ drive : ClassicationScenario 1- Prepare Data : https://visualreview.cozycloud.cc/#/2/47/1695
  • ❌ drive : ClassicationScenario 2- Classification actions on file : https://visualreview.cozycloud.cc/#/2/30/1696
  • ❌ drive : ClassicationScenario 3- Trash and Restore : https://visualreview.cozycloud.cc/#/2/48/1697
  • @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ photos : AlbumSharingScenario 2- Go to public link and download files : https://visualreview.cozycloud.cc/#/1/6/1700
  • @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ photos : PhotosUpload 1- Upload Photos : https://visualreview.cozycloud.cc/#/1/9/46
  • @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ drive : ClassicationScenario 1- Prepare Data : https://visualreview.cozycloud.cc/#/2/10/50
  • ❌ drive : ClassicationScenario 2- Classification actions on file : https://visualreview.cozycloud.cc/#/2/12/51
  • ❌ drive : ClassicationScenario 3- Trash and Restore : https://visualreview.cozycloud.cc/#/2/13/53
  • ❌ drive : Search : https://visualreview.cozycloud.cc/#/2/5/59
  • @Crash-- Crash-- changed the title [WIP] Fix/quick view Fix/quick view May 27, 2019
    @Crash-- Crash-- requested a review from y-lohse May 27, 2019 07:53
    src/drive/web/modules/drive/SharingsContainer.jsx Outdated Show resolved Hide resolved
    src/drive/web/modules/drive/SharingsContainer.jsx Outdated Show resolved Hide resolved
    src/drive/web/modules/drive/SharingsContainer.jsx Outdated Show resolved Hide resolved
    src/drive/web/modules/navigation/FileExplorer.jsx Outdated Show resolved Hide resolved
    src/drive/web/modules/navigation/RealtimeFiles.jsx Outdated Show resolved Hide resolved
    src/lib/promise.js Outdated Show resolved Hide resolved
    @Crash-- Crash-- changed the title Fix/quick view [WIP] Fix/quick view May 28, 2019
    folder,
    fileCount: folder.contents.meta.count || 0,
    files: folder.contents.data
    })
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    IMHO this kind of thing should be done in the reducer. It should ignore the success if it is for the wrong id.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    You absolutely right! But we have a "special" reducer in drive. We splited / sliced our reducer and state. So for instance here https://github.com/cozy/cozy-drive/blob/master/src/drive/web/modules/navigation/duck/reducer.js#L197 I can't have a direct access to the "global" state since in this method I'm scoped. So no acess state.openedFolderId :(

    Maybe I missed something to get access to it. I'm very open to suggestion here !

    @cozy-bot
    Copy link

    Visual Review - Please review screenshots, then restart build.

  • ❌ photos : PhotosUpload 1- Upload Photos : https://visualreview.cozycloud.cc/#/1/1/684
  • ❌ photos : CreateFullAlbumScenario 1- Create Full Album : https://visualreview.cozycloud.cc/#/1/13/691
  • ❌ photos : PhotosDelete 1- Delete Photos : https://visualreview.cozycloud.cc/#/1/14/694
  • @Crash-- Crash-- merged commit b1ac56f into master Jul 19, 2019
    @Crash-- Crash-- deleted the fix/QuickView branch July 19, 2019 08:31
    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

    6 participants