Conversation
Work Left To Do
Considerations
|
Demo Screen Recordings
iPhone97-demo-iphone-simulator-github.mp4iPad97-demo-ipad-simulator-github.mp4 |
jancborchardt
left a comment
There was a problem hiding this comment.
Very nice! :) Small improvement: Rather than the icon-only buttons on the bottom, it would be nice if they have text. Probably "Add existing account" (is that the proper wording?) and "Log in with QR code".
Alternative Login Method Design ChangesTo have buttons with labels, the layout must be adapted to account for localization problems due to varying text lengths. iPhone SE (3rd Gen) in Portrait OrientationiPhone 16 Pro Max in Portrait OrientationiPad in Landscape OrientationiPad in Portrait Orientation |
mpivchev
left a comment
There was a problem hiding this comment.
Localization needs to be added
|
Very nice indeed! I just have two pieces of feedback about wording:
*We should make sure capitalization is consistent across our iOS apps. I'm personally pro title case as it is what the platform guidelines recommend, but right now it's completely inconsistent. It would be nice to agree on one and coordinate changing all strings to match it. |
What part are you referring to exactly, @kra-mo? I assume you only mean the single sheet title, right? I updated it.
Platform conventions always beat corporate practice in my opinion. Out of experience, one will loose with the latter by reaching inevitable limitations at some point. You should probably discuss this with @jancborchardt who has the opposite opinion. |
The files and talk apps are inconsistent already, @mpivchev. Also, the master-detail navigation pattern used in the files app is conflicting with the back and forth inside the web view, creating a two level back- and forth navigation. The sheet provides a conventional experience for the cancellable and modal process in front. It maintains the usual swipe down gesture to dismiss. It also is more consistent with the presentation on larger screens like iPad. I updated the sheet to not have a navigation bar anymore which only occupies screen space without any value. This also is a design decision to a large extend and if this is a serious concern, then we should rather discuss it in a larger round with the designers. |
Thanks, I think this looks great now. I will discuss with Jan then :) |
jancborchardt
left a comment
There was a problem hiding this comment.
Platform conventions always beat corporate practice in my opinion. Out of experience, one will loose with the latter by reaching inevitable limitations at some point. You should probably discuss this with @jancborchardt who has the opposite opinion.
@i2h3 just discussed with @kra-mo as well, and to be clear: I also think that platform conventions should be the primary thing followed. This is also why we refer the Apple HIG in our design guidelines, and why we say to use SF Symbols (rathr than Material Design icons), and the San Francisco font, etc. :)
|
@mpivchev @jancborchardt @kra-mo 4.4.0 build 2 available in TestFlight now: https://testflight.apple.com/join/m7C1rYuN |
|
@mpivchev @jancborchardt You need to review again and check for your requested changes to be implemented. I still need your approval before I can integrate this. |
…ews and moved it into the buildable folder for the associated target. Signed-off-by: Iva Horn <iva.horn@icloud.com>
…ode style enforcement for the future. Signed-off-by: Iva Horn <iva.horn@icloud.com>
…folder for the associated target. Signed-off-by: Iva Horn <iva.horn@icloud.com>
…dable folder for the associated target. Signed-off-by: Iva Horn <iva.horn@icloud.com>
…yboard. Signed-off-by: Iva Horn <iva.horn@icloud.com>
- Enabled generated asset symbol extensions. - Removed unnecessary build files from iOCNotes build target. - Renamed "Embed App Extensions" build phase of iOCNotes build target. - Updated build settings. - Enabled parallelization in command-line builds. Signed-off-by: Iva Horn <iva.horn@icloud.com>
Project: - Slightly refined SwiftLint configuration. Views and Business Logic: - Introduced central Store for global app state and as a persistence API. - Introduced CodeScanner dependency for easy QR code scanning in SwiftUI. - Added required views for login and compartmentalized important business logic into protocols which they conform to. - Added SwiftUI sheet for suggesting shared accounts which also pre-fill the username in the login flow. - Notes are synchronized automatically for the first time immediately after login without the need of a manual pull to refresh first. - Notes table view controller now observes the Store.isSynchronizing to present the synchronization activity accordingly. - Login features have been removed from settings. - Settings have been extended with displaying account information instead. - Added logout button with confirmation alert to app settings. - Removed obsolete and unused login scenes from storyboard. - Removed obsolete NCShareAccounts storyboard and code. - Removed obsolete code from SettingsTableViewController and SettingsView. Assets: - Introduced accent color in the new default asset catalog with the Nextcloud blue. - Introduced brand logo in the new default asset catalog. Signed-off-by: Iva Horn <iva.horn@icloud.com>
Signed-off-by: Iva Horn <iva.horn@icloud.com>
Signed-off-by: Iva Horn <iva.horn@icloud.com>
It needs to be maintained manually and is redundant with the much more accessible display in the app settings. Signed-off-by: Iva Horn <iva.horn@icloud.com>
Signed-off-by: Iva Horn <iva.horn@icloud.com>
Signed-off-by: Iva Horn <iva.horn@icloud.com>
Signed-off-by: Iva Horn <iva.horn@icloud.com>
jancborchardt
left a comment
There was a problem hiding this comment.
I don't have access to an iOS device until Friday late eve – but since @kra-mo said that it looks great now, I'd consider it approved from design side. :)
|
@jancborchardt What I meant was pressing the button to now approve this pull request because GitHub blocks it otherwise. 😄 As visible in the checks, you requested changes. I could probably dismiss your review but it would be incorrect. You just need to approve on GitHub this time. |
jancborchardt
left a comment
There was a problem hiding this comment.
@i2h3 here you go, nice work! :)
|
Now only @mpivchev needs to review again and approve, hopefully. 🙂 |
mpivchev
left a comment
There was a problem hiding this comment.
LGTM :) Just one question







This resolves #97.