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

Provider views rewrite (.files, .folders => .partialTree) #5050

Draft
wants to merge 120 commits into
base: main
Choose a base branch
from

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented Mar 29, 2024

fixes #5000, fixes #4609, fixes #4414, fixes #5063

Description

  • enables indeterminate checkmark states

  • enables folder caching

  • fixes the issue where Unsplash was only loading one page

  • removed two-way binding in onFirstRender (not a backwards-compatible change, but only for people with custom providers)

  • addresses this Remote file paths #4537 (comment), absDirPath and relDirPath are injected in a single place

  • nOfSelectedFiles is as smart as it gets now

  • fixes the UI issue where shift-clicking files gets them highlighted:

  • fixes the way shift-clicking works in grid providers such as Instagram/Unpslash

  • makes the GoogleDrive's VIRTUAL_SHARED_DIR checkable (see this discussion https://transloadit.slack.com/archives/C0FMW9PSB/p1714529071856209)

TODO

  • When reviewers agree to the new "Not adding ${filesAlreadyAdded.length} files because they already exist" notification messages, properly translate them in the locales.
  • To Evgenia - don't forget to remove loadAllFiles: false & limit: 5 from providers when preparing for a review
  • To Evgenia - don't forget to remove console.logs

Future TODOs

  • I don't like how restrictions are handled currently. I think individual-file restrictions should disable the checkboxes (like they do now); but aggregate restrictions should only be shown in the footer. As a benefit, this can be made consistent with what it looks like when we're dropping files from our local file system.

Notes to reviewers

  • I made deliberate effort not to touch the folder structure at all (for ease of reviewing & because we didn't set our minds on which one we'd prefer yet)
  • This PR actually reduces the number of lines by a few hundred lines - the increase is due to the test file I added

GoogleDrive
- travelling down into folders works
- checking a file works
- breadcrumbs DONT work
Moves stuff closer to where it's used, prevents props drilling
When it's set to 5 pages you have to reduce the browser window to make it scrollable
@lakesare
Copy link
Contributor Author

lakesare commented May 8, 2024

@nqst, as per our call - I will describe my questions about restrictions UI.
I think this is best done as a series of situations.

Situation 1

const restrictions = { maxNumberOfFiles: 3 }

I check a folder that has 4 files in it, but we don't yet know about it.

image

I open that folder, and we find out that it has 4 files inside!

image

What should happen here.

My ideas:

  • It shows those 4 files as selected, but in the footer we show "Uppy only allows 3 files, but 4 files were selected"
    • it stills allows us to click the SELECT (4) button, it will tell us to reduce the number of files in the next screen [explanation: in the next screen we will actually be seeing all files we have ever added, and we will see file sizes - it will be easier for the user to select which files are excessive]
    • OR it doesn't allow us to click the SELECT (4)
  • It doesn't show those 4 files as selected - it only selects the first 3 files.
    The issue with this option is - what should we do with the parent folder, should we update it to "partially checked" folder state?
  • It shows those 4 files as selected, but, when we click SELECT (4), we get some error notification

Situation 2

const restrictions = { maxNumberOfFiles: 2000 }

I check a folder, which has thousands of files in it. I don't yet know how many - we will discover it upon scrolling.

image

I open it, and all files are marked as checked.

image

As I keep scrolling, we stumple upon the 2000ths file.
What should happen next? Should the 2001st file NOT get marked as checked? Again, what should happen to the parent folder - should it update its state to "partially checked"?

Situation 3

const restrictions = { maxNumberOfFiles: 3 }

I check some folders. I never open any of these folders, and we do not know how many files there are.

image

I click "Select (2)". There are 1000 files inside of those folders, which violates our restrictions.
What should happen?

My ideas:

  • Only add the first 3 files we fetch, and show the notification to the user "Not adding 997 files because they didn't pass restrictions"
  • Do NOT add any files - show user the error in the footer, saying "Uppy only allows 3 files, but 4 files were selected"
  • Do add ALL 1000 files without any warnings - and show the "Uppy only allows 3 files, but 1000 files were selected" error in the footer of the next page.

Situation 4

const restrictions = { maxNumberOfFiles: 3 }

I check 1 file.

image

and then I shift-click to the 5th file

image

What should happen?

Situation 5

const restrictions = { maxNumberOfFiles: 3 }

I check 3 files.

image

What should happen, should other checkboxes grey out like they currently do?
Or, if we add the error message in the footer - should we let the user select the 4th file and only then show them the error?


How to play with it locally

You can checkout this branch to play with it if it helps! I set GoogleDrive's page limit to 10 files so that it's easy to test both the scrolling behaviour and opening the folder behaviour.

@Murderlon
Copy link
Member

Won't we eliminate a whole set of problems and a lot of code complexity by simply not allowing folders to be checked? Instead we offer a "select all" checkbox which checks as many as possible within the current folder up until the limit with the current sorting?

I feel like we starting to over-engineer this, causing many different UI states which are more confusing to the user to figure out than the likelihood of them wanting to select entire folders. Ideally we design for the 80% use case, and I think that's selecting individual files.

@lakesare
Copy link
Contributor Author

lakesare commented May 8, 2024

@Murderlon, disagreed, people clearly use folder selection, see all the discussions about relativePath vs absolutePath.
And if we disable this, we'll make downloading large number of files outright impossible.

Also - I regularly use folder selection in the wild myself, for example when you're uploading your assignment on university websites, they expect a particular folder structure to be kept.

@mifi
Copy link
Contributor

mifi commented May 8, 2024

Sounds like it's getting a bit complicated now. Maybe for simplicity of implementation we could:

  • Allow the user to check however many files/folder they want without any restrictions in the UI
  • Once the user clicks "select (X) files", then we recurse through all selected directories and count all files. Once we exceed the limit, the we do NOT add any files, but instead show an error message "Uppy only allows X files, but Y files were selected". then the user can go back and deselect files/folders.

@Murderlon
Copy link
Member

First want to say that thanks for putting in big effort of cleaning this up AND writing a lot tests ✨


If we want to keep folder selection, I would follow these options

Situation 1: max files set to 3, you select a folder & then open it, it has 4 files.
--> It shows those 4 files as selected, but, when we click SELECT (4), we get some error notification

Situation 2: identical to situation one from my understanding.

Situation 3: I click "Select (2)". There are 1000 files inside of those folders, which violates our restrictions.
--> I think once again the same? If you open them one by one, you'll see "select {number}" but once you click it none are added and you see an error

Situation 5: should checkboxes be disabled once you reach the limit.
--> I don't think we have to to simplify it + we kind of have to, as we allow selecting beyond the limit in the other examples.


I do think it's essential however to break this PR up into smaller PRs. I understand this made sense to experiment and find a direction, but once we have that I expect this to be four PRs or so. For instance:

  1. shift click fixes
  2. removing the View class
  3. Adding basic partialTree structure
  4. refactor of ProviderView/SearchProviderView.

It will be too hard to review otherwise I'm afraid.

@lakesare
Copy link
Contributor Author

lakesare commented May 9, 2024

@mifi Sounds like it's getting a bit complicated now. Maybe for simplicity of implementation we could:

  1. Allow the user to check however many files/folder they want without any restrictions in the UI
  2. Once the user clicks "select (X) files", then we recurse through all selected directories and count all files. Once we exceed the limit, the we do NOT add any files, but instead show an error message "Uppy only allows X files, but Y files were selected". then the user can go back and deselect files/folders.

I like this idea, however consider the following situation.
Currently, when we have const restrictions = { maxNumberOfFiles: 1 } and we check a single file, all the other files become unselectable:

image

If we implement "restriction validation only on Select (x)", the user will have no way to discover whether they have selected too many files - imagine having selected 10 files only to discover 3 files were allowed. This is especially important for the maxTotalFileSize restriction, because file sizes are harder to predict. Also - I believe const restrictions = { maxNumberOfFiles: 1 } is a pretty frequent use case, and all the files greying out upon the selection of a single file is attractive.

I agree it's the simplest implementation possible, in fact what you're suggesting is exactly what I decided to do a week or so ago in order to "ship this PR, and maybe think through better restrictions system in further PRs".

But I think the situation I just described is a serious downside, so I decided to ask for Alex's help in coming up with the "ideal restrictions system".
Once we know what an "ideal restrictions system" would look like, it will be easier to see what I must implement in this PR, and what we can leave to others.


One variant of what you're suggesting is "allow the user to check however many files/folders they want without any restrictions in the UI; but show the error in the footer next to Select (x) if they violate some restrictions".

This solves the issue I just described; but does remove the attractive greying-out of the checkboxes (we will only show the error when the user checks 2 files, we won't see any feedback when the user checks 1 file).

@nqst
Copy link
Contributor

nqst commented May 9, 2024

I agree we should keep it simple. I don't think we should be engineering an "ideal" system right now. We could think about some magic solution(s), but it seems it's going to be prone to bugs and always have edge cases which does more harm than good both for us and the users.

So, I mostly agree with Merlijn's suggested solutions and Mikael's idea to:

Allow the user to check however many files/folder they want without any restrictions in the UI

It's not a significant issue to allow users to select files and then show an error if restrictions aren't respected. However, I have some ideas that can hopefully make the entire experience smoother:

  1. We could display the restriction message when the user is selecting files as well. The initial message about restrictions on the home screen can be easily ignored, but having the message visible in the file browser may help prevent incorrect actions.

  2. I like how nicely we currently support the case when the user can select only one file, and I agree that would be good to keep this. My idea is to keep this behavior only for { maxNumberOfFiles: 1 }, similar to a regular <input type="file"> without the multiple attribute set. Therefore, if the widely-used single-file mode is on, we'll disable the rest of the files when one file is selected. But if a more advanced restriction mode is enabled, let's not over-engineer it — allow users to select what they want, and then show an error if something isn't right.

  3. The error message should be more clear. When testing, I saw this:

    It doesn't look good to me. The messages contradict each other, and the error doesn't look like an error. It would be great to improve this.

What do you folks think?

@mifi
Copy link
Contributor

mifi commented May 9, 2024

One variant of what you're suggesting is "allow the user to check however many files/folders they want without any restrictions in the UI; but show the error in the footer next to Select (x) if they violate some restrictions".

This solves the issue I just described; but does remove the attractive greying-out of the checkboxes (we will only show the error when the user checks 2 files, we won't see any feedback when the user checks 1 file).

I agree. Maybe we could even grey out checkboxes if the user has reached the limit. It will not be 100% correct (because the user might have selected at least one folder which contains more files), however it's an optimistic guess. The only problem I see (with greying out optimistically based on number of checked checkboxes) is that if the user checks a folder, but later it turns out the folder is empty, then the folder was counted as 1 file towards the limit, but in reality there are no files inside, so the user could actually have added 1 more file. Or maybe we should count checked folders as 0 towards the limit.

@lakesare
Copy link
Contributor Author

lakesare commented May 10, 2024

@nqst,

  1. Making this work for { maxNumberOfFiles: 1 } is not significantly easier than making it work for { maxNumberOfFiles: 200 }. We get into the same "5 situations" I described above.
  2. Agreed error messages have to be reworked.

@lakesare
Copy link
Contributor Author

lakesare commented May 10, 2024

@nqst: I agree we should keep it simple. I don't think we should be engineering an "ideal" system right now. We could think about some magic solution(s), but it seems it's going to be prone to bugs and always have edge cases which does more harm than good both for us and the users.

"Ideal" system doesn't necessarily bring complexity in implementation with it!
We don't have to think it through now however, I'm friendly towards the idea of implementing a bare minimum solution for providers in this PR, and drawing upon this blank slate later, after we decide on a better restrictions system.

Let's think about what would be the minimal restrictions system in this PR that we are willing to accept.
The easiest thing to implement would indeed be @Murderlon and @mifi -suggested "we pretend there are no aggregate restrictions" solution.

I will describe to you 2 solutions that are equally easy to implement.
In both of these solutions we are not greying out files (to be more precise - we are greying out files with individual restrictions, but not files with aggregate restrictions).

Solution 1: error on click

const restrictions = { maxNumberOfFiles: 3 }
  1. User checks as many files as they like, there is no feedback telling them they are doing something wrong. User checks 5 files.
  2. User clicks "Select (X)"
  3. User sees the notification "Please select at most 3 files"
  4. User is still in the GoogleDrive interface - they are free to uncheck some files and try clicking "Select (X)" again.

Solution 2: error in the footer

const restrictions = { maxNumberOfFiles: 3 }
  1. User checks as many files as they like. User checks 5 files.

  2. In the footer, they see the "You can only select 3 files" error. The "Select (X)" button is disabled.

    image
  3. User unchecks 2 files. Error disappears.

    image

@nqst, @mifi, @Murderlon - do you find either of these solutions acceptable, and do you have a preference for one over the other?

@mifi
Copy link
Contributor

mifi commented May 10, 2024

if we are not greying out checkboxes once the limit has been reached, then i like solution 2 more

@Murderlon
Copy link
Member

Option 2 sounds good!

@nqst
Copy link
Contributor

nqst commented May 13, 2024

@lakesare I also like the second solution you proposed 👍

Making this work for { maxNumberOfFiles: 1 } is not significantly easier than making it work for { maxNumberOfFiles: 200 }. We get into the same "5 situations" I described above.

I think in the case of { maxNumberOfFiles: 1 }, we could apply Merlijn's idea to remove the folder selection. This change would immediately resolve situations 1-4, and make the UI clearer. As for situation 5, we can gray out remaining files after one file has been selected.

This means that the one-file-only scenario would have its own distinct behavior, but I believe this could be beneficial and less confusing for users.

What do you think?

@lakesare
Copy link
Contributor Author

lakesare commented May 13, 2024

@nqst,
I think if we have the "error in the footer", we don't need to disable folders to accompany the greying-out; "error in the footer" already deals with the 5 situations I described.

So, now we have the following choice.

Solution 1

  • show the error in the footer
  • DON'T grey out anything

Solution 2

  • show the error in the footer
  • additionally grey out files if we have { maxNumberOfFiles: 1 }

Solution 3

  • show the error in the footer
  • additionally grey out files if we have { maxNumberOfFiles: any } or { maxTotalFileSize: any }

Thing is - Solution 2 and Solution 3 are about equal in difficulty/overengineering, for both we need custom handling.
So, I would either go for Solution 1, or for Solution 3.

I think Solution 3 is already close to one variant of what I described as "ideal restriction systems".
But I feel we are locking ourselves into this option instead of thinking through the alternative "ideal restriction systems". One alternative would be what I think I described to you in our call - showing users the error in the footer; but letting them proceed with the selection, so that they can remove excessive files in the upload view.

I think that unless you think Solution 3 is a good option long-term, I should just implement Solution 1 in this PR, and leave further thinking on this topic to other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants