-
Notifications
You must be signed in to change notification settings - Fork 179
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
Theme override #4226
Theme override #4226
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good in general, but I'm a bit worried about the changes in tokens and how they affected the preview. Can we double check those are fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't these been reversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but we were using colors not from the theme... We need a design source for this anyway.
@amshakal can you help please? We need colors for this banner:
![image](https://private-user-images.githubusercontent.com/3940906/409037197-26163d29-6c1a-4422-be11-0afd783ae2e4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2NDQxODAsIm5iZiI6MTczOTY0Mzg4MCwicGF0aCI6Ii8zOTQwOTA2LzQwOTAzNzE5Ny0yNjE2M2QyOS02YzFhLTQ0MjItYmUxMS0wYWZkNzgzYWUyZTQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMTgyNDQwWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZGFkMjk4ZWE2MjBmY2Q0YmUxYjhmNzc3NGIyYTY5MDE5Nzg5YjRiZTMwNzg3NTg5MGZjMmU5MWEzYmVhNzE5NiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.os4hXuXLsx8vQJGQD-jcoo-_8Yq0cGpOPLMZj5KK_iE)
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmartinesp , Amsha will provide some design for this banner, but I do not think this should block this PR. Coming design will be handle in a specific PR. This PR just ensure that we are at least using semantic colors for this banner. Do you agree? If yes, I guess we can merge the PR except if you have other remarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not change this color to change it back later, but I don't think it should be blocking either, so it's ok to keep the change. I'll review the PR again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried these items now look less like they're in a disabled state, but I guess they are using the right tokens now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so
verticalAlignment = Alignment.CenterVertically, | ||
horizontalArrangement = Arrangement.Center | ||
) { | ||
Text( | ||
text = stringResource(id = R.string.screen_room_timeline_no_permission_to_post), | ||
color = MaterialTheme.colorScheme.onSecondary, | ||
color = ElementTheme.colors.textSecondary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Semantic colors for this banner, waiting for design input that can be handled later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added question and a possible issues with a template. Once the latter is fixed, it should be fine to merged this, thanks!
@@ -21,6 +21,7 @@ dependencies { | |||
api(projects.features.enterprise.api) | |||
implementation(projects.libraries.architecture) | |||
implementation(projects.libraries.matrix.api) | |||
api(libs.compound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these api
needed (here and in the api module)? Couldn't they just be implementation
since the designsystem module will already provide the compound dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right: b7dc2e3
@@ -19,7 +19,7 @@ fun ${NAME}View( | |||
Box(modifier, contentAlignment = Alignment.Center) { | |||
Text( | |||
"${NAME} feature view", | |||
color = MaterialTheme.colorScheme.primary, | |||
color = ElementTheme.colors.textprimary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a typo. textPrimary
with capital P
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, thanks! 9411cad
823485b
to
9411cad
Compare
|
Content
Give ability to override theme color at build time.
Get color from ElementTheme instead of MaterialTheme when possible.
Motivation and context
Allow Element Enterprise customization.
Screenshots / GIFs
Tests
Tested devices
Checklist