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

Add other Nextcloud apps to settings as suggestions #2530

Merged

Conversation

mpivchev
Copy link
Collaborator

No description provided.

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
…add-other-nextcloud-apps-to-settings-as-suggestions

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>

# Conflicts:
#	Nextcloud.xcodeproj/project.pbxproj
#	iOSClient/NCGlobal.swift
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
…add-other-nextcloud-apps-to-settings-as-suggestions

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>

# Conflicts:
#	Nextcloud.xcodeproj/project.pbxproj
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@mpivchev
Copy link
Collaborator Author

mpivchev commented Jul 13, 2023

@jancborchardt @marinofaggiana

Here is the new look on phone:

image

Untitled 2

And yes, I noticed that on iPhone 14s (with the new notches) the content does not have any padding (indicated with the red border). This is because we completely ignore the safe area when we render the cells. This is what we do to render them:

 override var frame: CGRect {
        get {
            return super.frame
        }
        set (newFrame) {
            var frame = newFrame
            let newWidth = frame.width * 0.90
            let space = (frame.width - newWidth) / 2
            frame.size.width = newWidth
            frame.origin.x += space
            super.frame = frame
        }
    }

Fixing this is out of scope of this ticket, but I will make a new one after this is finished.

@mpivchev
Copy link
Collaborator Author

Will also test iPad and post it here

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@mpivchev
Copy link
Collaborator Author

@jancborchardt This is iPad:

We can either increase the size of the icons or text, or reduce the max width.

image

image

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@mpivchev mpivchev marked this pull request as ready for review July 13, 2023 17:10
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Patch coverage: 7.36% and project coverage change: +0.90% 🎉

Comparison is base (8dbf5fc) 9.37% compared to head (6766167) 10.27%.

❗ Current head 6766167 differs from pull request most recent head 2650a51. Consider uploading reports for the commit 2650a51 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2530      +/-   ##
===========================================
+ Coverage     9.37%   10.27%   +0.90%     
===========================================
  Files          185      188       +3     
  Lines        26355    26354       -1     
  Branches      9827     9833       +6     
===========================================
+ Hits          2471     2709     +238     
+ Misses       23669    23417     -252     
- Partials       215      228      +13     
Files Changed Coverage Δ
Brand/NCBrand.swift 64.66% <ø> (ø)
.../Extensions/UINavigationController+Extension.swift 43.75% <0.00%> (ø)
iOSClient/More/Cells/BaseNCMoreCell.swift 0.00% <0.00%> (ø)
...OSClient/More/Cells/NCMoreAppSuggestionsCell.swift 0.00% <0.00%> (ø)
iOSClient/More/Cells/NCMoreUserCell.swift 0.00% <0.00%> (ø)
iOSClient/NCGlobal.swift 87.09% <ø> (+54.83%) ⬆️
iOSClient/More/NCMore.swift 2.56% <14.58%> (+0.68%) ⬆️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@mpivchev mpivchev force-pushed the 2460-add-other-nextcloud-apps-to-settings-as-suggestions branch from fe41b5a to 24e323c Compare July 14, 2023 08:50
@marinofaggiana
Copy link
Member

please @mpivchev rebase thx

@marinofaggiana marinofaggiana self-requested a review July 17, 2023 09:35
…add-other-nextcloud-apps-to-settings-as-suggestions
@mpivchev
Copy link
Collaborator Author

@marinofaggiana done. how is this design wise? @jancborchardt ping in case missed

@mpivchev
Copy link
Collaborator Author

@marinofaggiana I fixed the bugs with the missing sections, and from my tests everything was correct (including external sites section), but if you have time please test as well :)

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@marinofaggiana
Copy link
Member

@marinofaggiana I fixed the bugs with the missing sections, and from my tests everything was correct (including external sites section), but if you have time please test as well :)

good !
but the GUI with iPad and landscape Max I don't like .. try the first two in the left and the "more" on the right and reduce the hight of cell (section) if we really have to put it on at least let it be as bulky as possible :D

@mpivchev
Copy link
Collaborator Author

@marinofaggiana I fixed the bugs with the missing sections, and from my tests everything was correct (including external sites section), but if you have time please test as well :)

good ! but the GUI with iPad and landscape Max I don't like .. try the first two in the left and the "more" on the right and reduce the hight of cell (section) if we really have to put it on at least let it be as bulky as possible :D

That can't really work since it's a stack view

@marinofaggiana
Copy link
Member

@marinofaggiana I fixed the bugs with the missing sections, and from my tests everything was correct (including external sites section), but if you have time please test as well :)

good ! but the GUI with iPad and landscape Max I don't like .. try the first two in the left and the "more" on the right and reduce the hight of cell (section) if we really have to put it on at least let it be as bulky as possible :D

That can't really work since it's a stack view

will make some test

@marinofaggiana
Copy link
Member

@jancborchardt can be a valid alternative ?

  • touch on nextcloud icon up-right
  • open menu with talk, note, other

Simulator Screenshot - iPhone 14 - 2023-07-19 at 13 56 32

@jancborchardt
Copy link
Member

@marinofaggiana this is unfortunately not really discoverable – we had it look very similar years ago where we had only "Files ⏷" displayed in the header, and few people got that Nextcloud was more than other apps. The problem is the same no matter which kind of switcher we have. Directly showing the other apps is a great way to make it very obvious. :)

jancborchardt
jancborchardt previously approved these changes Jul 20, 2023
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@mpivchev design-wise, the solution in #2530 (comment) looks great to me. :)

@hannesfritz in case you have input regarding how it looks on iPad – basically a pick between stretching or max-width for the group.

@marinofaggiana
Copy link
Member

@mpivchev design-wise, the solution in #2530 (comment) looks great to me. :)

@hannesfritz in case you have input regarding how it looks on iPad – basically a pick between stretching or max-width for the group.

@mpivchev I prefer the maximum width at this point (iPad, max) ...

@mpivchev
Copy link
Collaborator Author

@marinofaggiana @jancborchardt I think currently it looks ok and keeping going back on forth on the design is a waste of time. Waiting for approval on the code.

@marinofaggiana
Copy link
Member

marinofaggiana commented Jul 25, 2023

@mpivchev Resolve conflicts and merge thanks!

…add-other-nextcloud-apps-to-settings-as-suggestions

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>

# Conflicts:
#	Nextcloud.xcodeproj/project.pbxproj
Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@mpivchev mpivchev force-pushed the 2460-add-other-nextcloud-apps-to-settings-as-suggestions branch from 756367e to 50f06b3 Compare July 25, 2023 12:47
@mpivchev
Copy link
Collaborator Author

mpivchev commented Jul 25, 2023

@marinofaggiana Added the flag + reduce complexity of section code. Tested with flag to true + external site menu and it seems to work fine.

@Ivansss @tobiasKaminsky NCBrand has a new flag disable_show_more_nextcloud_apps_in_settings that shows or hides the more apps section. Default is false. Should be set to true for Brand releases in brander etc.

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

All good design-wise! :)

Copy link
Member

@marinofaggiana marinofaggiana Jul 27, 2023

Choose a reason for hiding this comment

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

the showMoreAppsSection must be moved in NCBrand, as disable_NextcloudApps

Signed-off-by: Milen Pivchev <milen.pivchev@gmail.com>
@marinofaggiana
Copy link
Member

please make a rebase

…add-other-nextcloud-apps-to-settings-as-suggestions
@@ -449,7 +449,7 @@ class NCGlobal: NSObject {
var capabilityExternalSites: Bool = false
var capabilityGroupfoldersEnabled: Bool = false // NC27

// MORE APPS
// MORE NEXTCLOUD APPS
let talkSchemeUrl = "nextcloudtalk://"
let notesSchemeUrl = "nextcloudnotes://"
let talkAppStoreUrl = "https://apps.apple.com/de/app/nextcloud-talk/id1296825574"
Copy link
Member

Choose a reason for hiding this comment

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

why use the address apple ?
use the app scheme, if do not open you can open the "https://www.apple.com/us/search/nextcloud?src=globalnav"

example
// nextcloudtalk://open-conversation?server={serverURL}&user={userId}&withRoomToken={roomToken}
if metadata.name == NCGlobal.shared.talkName {
let pathComponents = metadata.url.components(separatedBy: "/")
if pathComponents.contains("call") {
let talkComponents = pathComponents.last?.components(separatedBy: "#")
if let roomToken = talkComponents?.first {
let urlString = "nextcloudtalk://open-conversation?server=(appDelegate.urlBase)&user=(appDelegate.userId)&withRoomToken=(roomToken)"
if let url = URL(string: urlString), UIApplication.shared.canOpenURL(url) {
... bla bla bla
return
}
}
}
}

Simulator Screenshot - iPhone 14 - 2023-07-27 at 11 14 34

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I check if the app is already installed, if not them we open the app store page.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but this is the page open in emulator (Talk)

Copy link
Member

Choose a reason for hiding this comment

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

the use of app scheme nextcloudtalk:// avoids these problems

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I use, check the code. The app store does not open in the simulator, you have to test on actual device.

Copy link
Member

@marinofaggiana marinofaggiana Jul 27, 2023

Choose a reason for hiding this comment

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

Understand but this
https://github.com/nextcloud/NextcloudKit/blob/a5bad2b6238ac25b3a59c9e7d91826b4f74c16c3/Sources/NextcloudKit/NKShareAccounts.swift#L120
works in simultor and not and do not required the link to apple

let talkSchemeUrl = "nextcloudtalk://"
let notesSchemeUrl = "nextcloudnotes://"

we have already all so this, so you can use thappstorelink for open the browser "https://www.apple.com/us/search/nextcloud?src=globalnav" (works with emulator)

@marinofaggiana marinofaggiana self-requested a review July 27, 2023 13:40
@marinofaggiana marinofaggiana merged commit 50f5df2 into develop Jul 28, 2023
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the 2460-add-other-nextcloud-apps-to-settings-as-suggestions branch July 28, 2023 08:24
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.

3 participants