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

feat: Multiselect behaviour #1311

Open
wants to merge 21 commits into
base: feature-externalLinks
Choose a base branch
from

Conversation

adrien-coye
Copy link
Contributor

@adrien-coye adrien-coye commented Sep 27, 2024

This PR makes multi select work with a public share.
Only one action, Download all is available in this mode.
Select all is also working with a dedicated route.

Feature branch #1306

IMG_3400

@adrien-coye adrien-coye force-pushed the externalLinks-downloadFiles branch from 4061345 to 6c99ba0 Compare October 23, 2024 06:53
@adrien-coye adrien-coye force-pushed the externalLinks-multiSelect branch from 9651af4 to 0fe23be Compare October 23, 2024 07:16
Base automatically changed from externalLinks-downloadFiles to feature-externalLinks October 23, 2024 12:00
Copy link
Contributor Author

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

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

Self assesment

@adrien-coye adrien-coye force-pushed the externalLinks-multiSelect branch from 8c33c37 to 7054054 Compare November 27, 2024 06:31
@adrien-coye adrien-coye force-pushed the externalLinks-multiSelect branch from d6beea0 to 198356b Compare November 29, 2024 16:22
@adrien-coye adrien-coye force-pushed the externalLinks-multiSelect branch from 198356b to 6c88f15 Compare November 29, 2024 16:27
@adrien-coye adrien-coye force-pushed the externalLinks-multiSelect branch from 308f44c to b7cfac7 Compare December 4, 2024 09:16
@adrien-coye adrien-coye marked this pull request as ready for review December 4, 2024 09:29
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann left a comment

Choose a reason for hiding this comment

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

Even if viewControllerDismissable is a protcol and is weak. I don't think it is a good practice to keep a reference to the ViewController from the ViewModel.

The VM shouldn't control the ViewController. However you indeed need to callback your VC if you want to dismiss it.
In the code base I see two examples:

  • Implement the logic inside the VC (see SearchViewController)
  • Using a callback. It's more work but maybe more appropriate

@adrien-coye
Copy link
Contributor Author

@PhilippeWeidmann Yeah I agree, I felt it was not perfect while doing it. Yeah I can use a callback there.

Copy link

sonarcloud bot commented Dec 11, 2024

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.

2 participants