Skip to content
This repository was archived by the owner on Jun 7, 2020. It is now read-only.

Comments

[NEW] Adding the ability to theme#1602

Merged
rafaelks merged 156 commits intoRocketChat:developfrom
Sameesunkaria:themeable-rc
Jun 14, 2018
Merged

[NEW] Adding the ability to theme#1602
rafaelks merged 156 commits intoRocketChat:developfrom
Sameesunkaria:themeable-rc

Conversation

@Sameesunkaria
Copy link
Contributor

@Sameesunkaria Sameesunkaria commented Apr 25, 2018

@RocketChat/ios

This PR will focus on adding the ability to theme the application. The theme can be applied under Preferences > Theme. Currently there are three themes on the app, "Light", "Dark" and "Black".

A developer's guide for navigating these changes has been provided below.

Theme

A theme has been described as a class, containing the key colors and attributes which are required to theme the application.

Theme code

Adapting a view for the theme

UIView conforms to the protocol, ThemeProvider. This protocol has a read-only variable var theme: Theme? { get }. The value of this variable is determined by its superview. In case a superview does not exist, the value is taken from the ThemeManager.

To color the UI components based on the selected theme, override the applyTheme method on UIView. This method comes from the conformance to the Themeable protocol. Here you can set the colors on the UI components. This is usually done in an extension. Accessing the theme property, will enable you to use the key colors associated with that theme.

The base implementation of applyTheme automatically sets the background color of the view, and calls applyTheme on all of its subviews.

Here's an example showing how the theme was applied to ChatMessageAudioView.

Themeable view example

Note: It is recommended that you call super whenever overriding applyTheme.

A basic implementation for most of the UIKit view components has already been provided in ThemeableViews.swift.

Exempting a view from theming

A view and all of its subviews can be easily exempted from theming by overriding the theme property and returning nil.

override var theme: Theme? { return nil }

This behaviour can also be adobted from within the Interface Builder, by setting the custom class for the view to NotThemeableView.

Applying a theme

For the theme to take effect, it is required that the view controller or the view add themselves as an observer by calling the addObserver(_:) method on ThemeManager.

ThemeManager.addObserver(self)

ThemeManager expects that the observers conform to the protocol, Themeable. This protocol contains one required method, applyTheme. By default, calling this method on a UIView will theme that view and all of its subviews. And, calling this method on a UIViewController will pass this call to its view.

Using ThemeManager.addObserver(_:), automatically calls the applyTheme method on the object being added. The applyTheme method is also called on all of the observers when the theme changes.

This behaviour has already been implemented for BaseViewController and BaseTableViewController. Any subclass of the aforementioned view controllers are not required to call addObserver(_:) so long as they call super in viewDidLoad.

NOTE: ThemeManager does not hold a strong reference to the observer, so it is not required to the remove the observer from the ThemeManager.

The addObserver(_:) method only calls applyTheme once. This will make it so that all of the subviews added after calling the addObserver(_:) methods, will not be themed.

Updating the view after adding new subviews

You are required to call applyTheme on any of the subviews being added to the view, after the initial call to addObserver(_:).

The whole view is not required to be themed. The applyTheme method should only be called on the newly added subview.

UITableView, UICollectionView & UINavigationBar

UITableView, UICollectionView and UINavigationBar are already extended to accept new subviews and automatically call applyTheme on them.

Dealing with Navigation Bars

Apply theme method on the UINavigationBar will be automatically called when calling the applyTheme method on a BaseViewController or a BaseTableViewController. This behavior can be turned off by either overriding the theme property on the UINavigationBar, or using a NotThemeableNavigationBar.

This can be done inside the Interface Builder by setting the class of the UINavigationBar to NotThemeableNavigationBar.

Screenshots

Chat

Light theme Dark theme Black theme

Preferences

Theme preferences

@objc func applyTheme(_ theme: Theme)
}

// TODO: These do not belong here

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (These do not belong here). (todo)

view.clipsToBounds = true
}

// TODO: This does not belong here

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (This does not belong here). (todo)

var window: UIWindow?
var notificationWindow: UIWindow?

// TODO: This does not belong here

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (This does not belong here). (todo)

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #1602 into develop will increase coverage by 0.99%.
The diff coverage is 59.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1602      +/-   ##
==========================================
+ Coverage     34.5%   35.5%   +0.99%     
==========================================
  Files          283     292       +9     
  Lines        13318   13621     +303     
==========================================
+ Hits          4596    4836     +240     
- Misses        8722    8785      +63
Impacted Files Coverage Δ
...lers/Notification/NotificationViewController.swift 47.05% <ø> (+2.35%) ⬆️
...ences/Profile/EditProfileTableViewController.swift 29.31% <ø> (ø) ⬆️
...es/ChangeAppIcon/ChangeAppIconViewController.swift 0% <ø> (ø) ⬆️
...ences/Profile/NewPasswordTableViewController.swift 0% <ø> (ø) ⬆️
.../Preferences/Language/LanguageViewController.swift 0% <ø> (ø) ⬆️
...es/Web Browser/WebBrowserTableViewController.swift 0% <ø> (ø) ⬆️
...ollers/Preferences/PreferencesViewController.swift 0% <ø> (ø) ⬆️
.../Views/Cells/Chat/ChatMessageUnreadSeparator.swift 0% <0%> (ø)
...t/Controllers/Chat/MembersListViewController.swift 0% <0%> (ø) ⬆️
...Chat/Views/Form/Cells/TextFieldTableViewCell.swift 0% <0%> (ø) ⬆️
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25874b7...4892ee0. Read the comment docs.

}
}

// TODO: The add/insertSubview methods should not be overridden for UICollectionView,

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (The add/insertSubview methods ...). (todo)

static let defaultFontColor = UIColor.darkGray
static let systemFontColor = UIColor.lightGray
static let failedFontColor = UIColor.lightGray
// TODO: Probably should not be changed here

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (Probably should not be changed...). (todo)

var theme: Theme? { get }
}

// TODO: These do not belong here

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (These do not belong here). (todo)

Podfile.lock Outdated
PODFILE CHECKSUM: 0af4673565909cbfda00f61124b8286f6433aff3

COCOAPODS: 1.5.3
COCOAPODS: 1.5.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update your CocoaPods to 1.5.3?

@rafaelks
Copy link
Contributor

@Sameesunkaria I enjoyed very much playing with the themes, congrats on the development of it! 👏

Couple of issues I found while testing (on iOS 11.4):

simulator screen shot - iphone se - 2018-06-12 at 16 16 08

  1. Checkmark is gray, should be blue for light theme.

simulator screen shot - iphone se - 2018-06-12 at 16 16 11

  1. "Away" should be gray, not black for light theme.

simulator screen shot - iphone se - 2018-06-12 at 16 16 24

  1. Checkmark is gray, should be blue for light theme.

simulator screen shot - iphone se - 2018-06-12 at 16 16 29

  1. "open.rocket.chat" is using a different gray color than today. Let's keep the same as today please, to all other grays on the app.

simulator screen shot - iphone se - 2018-06-12 at 16 16 50

  1. This is not the gray we have today for light theme.

simulator screen shot - iphone se - 2018-06-12 at 16 17 00

  1. This is not the gray we have today (I'm talking about the last message text) for light theme.

simulator screen shot - iphone se - 2018-06-12 at 16 17 16

  1. You changed the colors of the messages to black. Let's use the same as we're using today please.

simulator screen shot - iphone se - 2018-06-12 at 16 21 12

  1. The status screen is like this for Dark & Black themes.

@Sameesunkaria
Copy link
Contributor Author

Sameesunkaria commented Jun 12, 2018

Fixes

  • Checkmark is gray, should be blue for light theme.
  • "Away" should be gray, not black for light theme.
  • Checkmark is gray, should be blue for light theme.
  • "open.rocket.chat" is using a different gray color than today. Let's keep the same as today please, to all other grays on the app.
  • This is not the gray we have today for light theme.
  • This is not the gray we have today (I'm talking about the last message text) for light theme.
  • You changed the colors of the messages to black. Let's use the same as we're using today please.
  • The status screen is like this for Dark & Black themes.

@Sameesunkaria
Copy link
Contributor Author

@rafaelks Addressed those concerns.

@rafaelks
Copy link
Contributor

@Sameesunkaria Looking great! Thanks a lot for the hard work on this one!

@rafaelks rafaelks merged commit 1bbc60e into RocketChat:develop Jun 14, 2018
@filipealva
Copy link
Contributor

@Sameesunkaria Just reviewed it post-merge, it's looking awesome! Thank you 🥇

@Sameesunkaria Sameesunkaria deleted the themeable-rc branch August 31, 2018 12:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants