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

Replace Style by Theme #403

Merged
merged 13 commits into from
Oct 5, 2021
Merged

Replace Style by Theme #403

merged 13 commits into from
Oct 5, 2021

Conversation

phloux
Copy link

@phloux phloux commented Sep 23, 2021

#395

Copy link
Contributor

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

I added some comments. I will play with your code to eventually approve it before considering these comments. Please wait for syncing with me before modifying this PR

@@ -197,33 +194,37 @@ - (void)viewDidLoad

- (void)userInterfaceThemeDidChange
{
[self updateWithStyle:self.currentStyle];
[self updateTheme];
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you think possible to restore here the Element-iOS code for this method userInterfaceThemeDidChange

Copy link
Contributor

Choose a reason for hiding this comment

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

I compared both implementations of this class SegmentedViewController
I think we should try to restore the Element-iOS implementation of this class, and test whether we really need customization. We may talk in direct together about this plan

@@ -75,37 +69,35 @@ - (void)viewWillAppear:(BOOL)animated

- (void)userInterfaceThemeDidChange
{
[self updateWithStyle:self.currentStyle];
[self updateTheme];
}

- (void)applyVariant2Style
Copy link
Contributor

Choose a reason for hiding this comment

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

This method applyVariant2Style should be removes, don't you think so?

@interface WebViewViewController () <Stylable>

@property (nonatomic, strong) id<Style> currentStyle;
@interface WebViewViewController ()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have the same remarks as I had on the SegmentedViewController class

self.leftButton.backgroundColor = theme.headerBackgroundColor
self.leftButton.setTitleColor(theme.headerTextPrimaryColor, for: .normal)
self.rightButton.backgroundColor = theme.headerBackgroundColor
self.rightButton.setTitleColor(theme.headerTextPrimaryColor, for: .normal)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan to use headerBackgroundColorand headerTextPrimaryColor here, I don't want to blocck this PR on this point, but we have to create an issue to introduce correct keys in the theme for this kind of button.
This work (adding new theme keys) should be planned with Element-ios team to improve Element-ios

Copy link
Contributor

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

@phlniji I approved this PR - I let you merge it into develop

We will consider my comments separately
I have observed during my test only one regression (that we will fix after your merge): the pink badge on highlighted notifications is white:
image
image

@phloux phloux merged commit 2f06c14 into develop Oct 5, 2021
@phloux phloux deleted the phlpro/theme branch October 5, 2021 08:20
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