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

Issue 173 grid view #176

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

hoangnguyen177
Copy link

@hoangnguyen177 hoangnguyen177 commented Feb 21, 2021

Description

As per request 173, grid view is added to dataset, folder and search.

The views are as followed:
gridview_dataset
gridview_folder
gridview_search

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have signed the CLA
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@max-zilla
Copy link
Contributor

Can you take a quick look at the marking files system? During testing, on grid view when I would select a file inside a dataset it would set Marked (2) instead of Marked (1). The list view still seemed to add 1 correctly, but there seems to be some weird behavior there. Can you try to recreate? This is used for bulk file operations.

Screen Shot 2021-03-29 at 2 36 12 PM

A lot of the changes are looking good overall!

@tcnichol tcnichol self-requested a review May 17, 2021 20:26
@lmarini
Copy link
Member

lmarini commented Sep 10, 2021

@bodom0015 can you help review this? Thanks!

Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

Took a look at this PR and the tile-mode features seem to work as intended.

@max-zilla previously mentioned a bug with the Marked Files counter that appears to still be present. I can confirm that this behavior does not manifest on develop or master, and that it now affects both the grid and tile views. I am a bit confused tho, since I can't see where this PR touches that add/remove logic specifically...

And because this PR does not touch that code, I can't seem to make proper suggestions where the fix is needed :/

@lmarini @robkooper should this issue be addressed on this PR before merging? Or could this perhaps be merged and we can follow-up with a fix for this bug?


Digging into the underlying bug, it looks like the problem comes from the use of string/split to store these IDs as a cookie string. When the cookie is "empty", the value is somehow being stored a pair of double-quotes ("\"\"") instead of an empty string (""). For example, upon first click the cookie value is set to "\"\",60c2cbf56aec65b8de9a1775", and the leading pair of quotes causes an incorrect length of 2 files.

I suspect that this is because $.cookie appears to call JSON.stringify in the background on the stored value. Calling JSON.parse when reading the cookie back out seems to prevent some of the multi-quote issues, and starts to beg the question of if using a JSON array for the whole list might be preferable to string manipulation?

An implementation like the following would store as a JSON array (make sure to JSON.parse in the other 2 spots where the [email protected] cookie is read):

    function addFileToMarked(file_id) {
        var selected = $.cookie('[email protected]');
        if (selected) {
            // Parse JSON array and add new id
            var fileUUIDs = JSON.parse(selected);
            if (fileUUIDs.indexOf(file_id) === -1) {
                fileUUIDs.push(file_id);
            }
            // Set modified array as cookie
            $.cookie('[email protected]', fileUUIDs);
        } else {
            // Set array containing single id as cookie
            $.cookie('[email protected]', [file_id]);
        }

        $("a[data-id='"+file_id+"'] span.glyphicon").removeClass('glyphicon-plus');
        $("a[data-id='"+file_id+"'] span.glyphicon").addClass('glyphicon-ok');
        updateSelectedFileCount();
    }

    function removeFileFromMarked(file_id) {
        var selected = $.cookie('[email protected]');
        if (selected) {
            // Parse JSON array and attempt to remove id
            var fileUUIDs = JSON.parse(selected);
            var index = fileUUIDs.indexOf(file_id);
            if (index !== -1) {
                // if possible, remove the id (only set the cookie if it was modified)
                fileUUIDs.splice(index, 1);
                $.cookie('[email protected]', fileUUIDs, {expires: 14});
            }
        }

        $("a[data-id='"+file_id+"'] span.glyphicon").removeClass('glyphicon-ok');
        $("a[data-id='"+file_id+"'] span.glyphicon").addClass('glyphicon-plus');
        updateSelectedFileCount();
    }

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Hoang Nguyen
❌ hoangnguyen177


Hoang Nguyen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

6 participants