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
base: main
Are you sure you want to change the base?
Conversation
GoogleDrive - travelling down into folders works - checking a file works - breadcrumbs DONT work
Just makes it easier to read the structure of TagFile
(remove the dependency on provider, just pass a callback)
…l order in tests
…owser/> to avoid repetition
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
@nqst, as per our call - I will describe my questions about restrictions UI. Situation 1
I check a folder that has 4 files in it, but we don't yet know about it. I open that folder, and we find out that it has 4 files inside! What should happen here. My ideas:
Situation 2
I check a folder, which has thousands of files in it. I don't yet know how many - we will discover it upon scrolling. I open it, and all files are marked as checked. As I keep scrolling, we stumple upon the 2000ths file. Situation 3
I check some folders. I never open any of these folders, and we do not know how many files there are. I click "Select (2)". There are 1000 files inside of those folders, which violates our restrictions. My ideas:
Situation 4
I check 1 file. and then I shift-click to the 5th file What should happen? Situation 5
I check 3 files. What should happen, should other checkboxes grey out like they currently do? How to play with it locallyYou 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. |
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. |
@Murderlon, disagreed, people clearly use folder selection, see all the discussions about relativePath vs absolutePath. 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. |
Sounds like it's getting a bit complicated now. Maybe for simplicity of implementation we could:
|
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. 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. Situation 5: should checkboxes be disabled once you reach the limit. 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:
It will be too hard to review otherwise I'm afraid. |
I like this idea, however consider the following situation. 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 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". 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 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:
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:
What do you folks think? |
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. |
|
"Ideal" system doesn't necessarily bring complexity in implementation with it! Let's think about what would be the minimal restrictions system in this PR that we are willing to accept. I will describe to you 2 solutions that are equally easy to implement. Solution 1: error on click
Solution 2: error in the footer
@nqst, @mifi, @Murderlon - do you find either of these solutions acceptable, and do you have a preference for one over the other? |
if we are not greying out checkboxes once the limit has been reached, then i like solution 2 more |
Option 2 sounds good! |
@lakesare I also like the second solution you proposed 👍
I think in the case of 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? |
@nqst, So, now we have the following choice. Solution 1
Solution 2
Solution 3
Thing is - Solution 2 and Solution 3 are about equal in difficulty/overengineering, for both we need custom handling. I think Solution 3 is already close to one variant of what I described as "ideal restriction systems". 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. |
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
andrelDirPath
are injected in a single placenOfSelectedFiles
is as smart as it gets nowfixes 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
Not adding ${filesAlreadyAdded.length} files because they already exist
" notification messages, properly translate them in the locales.loadAllFiles: false
&limit: 5
from providers when preparing for a reviewconsole.log
sFuture TODOs
Notes to reviewers