[feature/v12 themeing] UI Theming updates for v12 #1191

hosy · 2023-03-15T13:39:51Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
Added an issue with details about all relevant changes in the iOS documentation repository.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
Added changelog files for the fixed issues in folder changelog/unreleased

…ios-app into feature/new-navigation

CLAassistant · 2023-03-15T13:39:57Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Matthias Hühne seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

hosy · 2023-03-15T13:56:16Z

(1)

sort header color may in navigation bar color

(2)

check file list colours:
secondary label color is currently using tint color

(3)

sidebar:
collection cell images using tint color (should use text colour)

(4)

Status bar color
currently not brandable

(5)

Action Alert
font colour is not visible (when using white tint colour)

(6)

sidebar foldable cell item has wrong indicator colour

(7)

try to set the tint colour to white

check Spaces
empty folder (button font color)

felix-schwarz

Overall, the look of all elements in the sidebar (incl. navigation bar) on the left should be distinct from the elements in the content area (again, including navigation bar) on the right, to provide the same visual cues as in stock iOS apps:

ownCloud.app	Files.app

felix-schwarz · 2023-03-17T16:01:05Z

ownCloudAppShared/Client/Collection Views/Cells/ActionCell.swift

@@ -190,6 +190,40 @@ class ActionCell: ThemeableCollectionViewCell {
 	}
 }

+
+class ActionListCell: UICollectionViewListCell, Themeable {


The implementation of ActionListCell and SavedSearchListCell appear to be identical in code - and the functionality they provide is also already provided by the existing ThemeableCollectionViewListCell, so it would make sense to use ThemeableCollectionViewListCell instead of ActionListCell and SavedSearchListCell classes.

felix-schwarz · 2023-03-17T16:12:08Z

ownCloudAppShared/Client/Collection Views/Cells/SavedSearchCell.swift

 			var content = cell.defaultContentConfiguration()
+            content.textProperties.color = Theme.shared.activeCollection.tableRowColors.labelColor
+            content.imageProperties.tintColor = Theme.shared.activeCollection.tintColor


Shouldn't these colors already be set via func updateConfiguration(using state: UICellConfigurationState)? If they aren't, I'd suggest to move that code into a method of the respective class and call it where it is needed (here and in updateConfiguration?), to avoid code duplication.

felix-schwarz · 2023-03-17T16:19:11Z

ownCloudAppShared/Client/Collection Views/Cells/DriveListCell.swift

@@ -236,6 +238,9 @@ extension DriveListCell {

 			content.text = title
 			content.image = icon
+
+            content.textProperties.color = Theme.shared.activeCollection.tableRowColors.labelColor
+            content.imageProperties.tintColor = Theme.shared.activeCollection.tintColor


Shouldn't these colors already be set via func updateConfiguration(using state: UICellConfigurationState) of ThemeableCollectionViewListCell? If they aren't, I'd suggest to move that code into a method of the respective class and call it where it is needed (here and in updateConfiguration?), to avoid code duplication.

felix-schwarz · 2023-03-17T16:21:44Z

...udAppShared/Client/Collection Views/CollectionViewCellProvider+StandardImplementations.swift

@@ -127,6 +127,9 @@ public extension CollectionViewCellProvider {
 			})

 			var content = cell.defaultContentConfiguration()
+
+            content.textProperties.color = Theme.shared.activeCollection.tableRowColors.labelColor
+            content.imageProperties.tintColor = Theme.shared.activeCollection.tintColor


Shouldn't these colors already be set via func updateConfiguration(using state: UICellConfigurationState) of ThemeableCollectionViewListCell? If they aren't, I'd suggest to move that code into a method of the respective class and call it where it is needed (here and in updateConfiguration?), to avoid code duplication.

felix-schwarz · 2023-03-17T16:23:37Z

ownCloudAppShared/Client/Collection Views/CollectionViewSection.swift

-
-						default: break
-					}
+                config.backgroundColor = Theme.shared.activeCollection.tableBackgroundColor


.plain, .grouped and .insetGrouped appearances should have a different look, for the sidebar to look in line with how sidebars look throughout the OS.

felix-schwarz · 2023-03-17T16:44:05Z

ownCloudAppShared/Client/Collection Views/Cells/AccountControllerCell.swift

@@ -305,11 +305,14 @@ class AccountControllerCell: ThemeableCollectionViewListCell {
 	}

 	open override func applyThemeCollection(theme: Theme, collection: ThemeCollection, event: ThemeEvent) {
+        titleLabel.applyThemeCollection(collection, itemStyle: .title, itemState: ThemeItemState(selected: self.isSelected))
+        detailLabel.applyThemeCollection(collection, itemStyle: .title, itemState: ThemeItemState(selected: self.isSelected))


This sets the detail label's color to the same color as the title label, whereas the detail label should have a lighter color.

felix-schwarz · 2023-03-17T16:45:57Z

ownCloudAppShared/Client/Collection Views/Cells/AccountControllerCell.swift

 		var backgroundConfig = UIBackgroundConfiguration.listSidebarCell()
 		backgroundConfig.cornerRadius = 10
-		backgroundConfig.backgroundColor = UIColor(white: 1.0, alpha: 0.8)
-
+        backgroundConfig.backgroundColor = collection.tableGroupBackgroundColor


The AccountControllerCell background color should be distinct from background colors of table / collection views.

felix-schwarz · 2023-03-17T16:48:35Z

...AppShared/User Interface/Browser Navigation Controller/BrowserNavigationViewController.swift

-		navigationView.applyThemeCollection(collection, itemStyle: .content)
+		navigationView.applyThemeCollection(collection, itemStyle: .defaultForItem)
+        contentViewController?.navigationItem.navigationContent.applyThemeCollection(collection)
+        if let items = contentViewController?.navigationItem.navigationContent.items(withIdentifier: "ios16-truncated-title-fix"), let label = items.0.first?.titleView as? UILabel {


That label is accessible more easily via the convenience accessor contentViewController?.navigationItem.titleLabel (implemented in UINavigationItem+Extension.swift).

felix-schwarz · 2023-03-17T16:52:44Z

ownCloudAppShared/Client/Collection Views/Cells/SavedSearchCell.swift

@@ -189,6 +189,36 @@ extension OCSavedSearch {
 	}
 }

+class SavedSearchListCell: UICollectionViewListCell, Themeable {


The implementation of ActionListCell and SavedSearchListCell appear to be identical in code - and the functionality they provide is also already provided by the existing ThemeableCollectionViewListCell, so it would make sense to use ThemeableCollectionViewListCell instead of ActionListCell and SavedSearchListCell classes.

felix-schwarz · 2023-03-17T16:54:55Z

ownCloudAppShared/Client/Search/Item Search/ItemSearchSuggestionsViewController.swift

 	}

 	required init?(coder: NSCoder) {
 		fatalError("init(coder:) has not been implemented")
 	}
+
+    deinit {
+        Theme.shared.unregister(client: self)


Good catch! No objections, only a comment: in V12 Theme.unregister(client:) no longer needs to be called. The deallocation of an object also automatically unregisters it as a Theme client.

felix-schwarz · 2023-03-31T07:08:08Z

Superseded by #1194

Matthias Hühne added 5 commits March 9, 2023 09:49

fixed missing theming parts

dd64ced

Merge branch 'feature/new-navigation' of https://github.com/owncloud/…

f68df51

…ios-app into feature/new-navigation

fixed missing theme views

41176d9

fixed eject button theming

d81d4af

new build number

60f756e

felix-schwarz requested changes Mar 17, 2023

View reviewed changes

felix-schwarz closed this Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature/v12 themeing] UI Theming updates for v12 #1191

[feature/v12 themeing] UI Theming updates for v12 #1191

hosy commented Mar 15, 2023

CLAassistant commented Mar 15, 2023

hosy commented Mar 15, 2023

felix-schwarz left a comment

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz Mar 17, 2023

felix-schwarz commented Mar 31, 2023

[feature/v12 themeing] UI Theming updates for v12 #1191

[feature/v12 themeing] UI Theming updates for v12 #1191

Conversation

hosy commented Mar 15, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

CLAassistant commented Mar 15, 2023

hosy commented Mar 15, 2023

(1)

(2)

(3)

(4)

(5)

(6)

(7)

felix-schwarz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-schwarz commented Mar 31, 2023