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.

Sign up for GitHub

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

Jump to bottom

fix: Verification icons dark mode #2580

Merged
merged 7 commits into from
Jan 16, 2024
Merged

fix: Verification icons dark mode #2580

merged 7 commits into from
Jan 16, 2024

Conversation

borichellow
Copy link
Contributor

What's new in this PR?

Issues

Verification icons (Proteus and MLS) were not adopted for Dark Mode.

Causes (Optional)

Just wasn't implemented

Solutions

Implement it

@borichellow borichellow self-assigned this Jan 12, 2024
borichellow
borichellow commented Jan 12, 2024
View reviewed changes
app/src/main/kotlin/com/wire/android/ui/common/WireDialog.kt
@@ -192,7 +192,6 @@ private fun WireDialogContent(
Text(
text = title,
style = MaterialTheme.wireTypography.title02,
modifier = Modifier.weight(1f)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caused title ignoring centerContent

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 12, 2024 •
edited
Loading

Test Results

743 tests  ±0   743 ✅ ±0   10m 6s ⏱️ +29s
102 suites ±0     0 💤 ±0 
102 files   ±0     0 ❌ ±0 

Results for commit 06876aa. ± Comparison against base commit b15453f.

♻️ This comment has been updated with latest results.

@AndroidBob
Copy link
Collaborator

Build 2524 failed.

@codecov Codecov
Copy link

codecov bot commented Jan 12, 2024 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (b15453f) 41.39% compared to head (06876aa) 41.40%.

Additional details and impacted files 
@@              Coverage Diff              @@
##             develop    #2580      +/-   ##
=============================================
+ Coverage      41.39%   41.40%   +0.01%     
  Complexity      1129     1129              
=============================================
  Files            361      361              
  Lines          13255    13259       +4     
  Branches        1742     1743       +1     
=============================================
+ Hits            5487     5490       +3     
- Misses          7236     7237       +1     
  Partials         532      532
Files Coverage Δ
.../android/ui/authentication/devices/model/Device.kt 95.00% <100.00%> (+0.55%) ⬆️
...ndroid/ui/settings/devices/SelfDevicesViewModel.kt 85.29% <60.00%> (-2.21%) ⬇️
...e/android/ui/home/conversations/model/UIMessage.kt 47.16% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

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

@github-actions GitHub Actions
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 2529 succeeded.

The build produced the following APK's:

saleniuk
saleniuk reviewed Jan 15, 2024
View reviewed changes
app/src/main/kotlin/com/wire/android/ui/common/VerifiedIcons.kt Outdated
Comment on lines 76 to 77
val iconId = if (isSystemInDarkTheme()) R.drawable.ic_certificate_valid_proteus_dark
else R.drawable.ic_certificate_valid_proteus
Copy link
Contributor

Choose a reason for hiding this comment

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

question: wouldn't just putting icon for dark theme inside drawable-night directory work the same but without the need to create these ifs and separate fields for iconResId and darkIconResId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dummy mistake 😅

@github-actions GitHub Actions
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 2562 succeeded.

The build produced the following APK's:

@borichellow borichellow requested a review from saleniuk January 16, 2024 09:28
@github-actions GitHub Actions
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 2576 succeeded.

The build produced the following APK's:

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 16, 2024
@borichellow borichellow enabled auto-merge January 16, 2024 14:56
@borichellow borichellow added this pull request to the merge queue Jan 16, 2024
@github-actions GitHub Actions
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

Merged via the queue into develop with commit 196381e Jan 16, 2024
14 of 15 checks passed
@borichellow borichellow deleted the fix/verification_icons_dark_mode branch January 16, 2024 15:27
@AndroidBob
Copy link
Collaborator

Build 2585 succeeded.

The build produced the following APK's:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saleniuk saleniuk saleniuk approved these changes

@mchenani mchenani mchenani approved these changes

@alexandreferris alexandreferris Awaiting requested review from alexandreferris

@MohamadJaara MohamadJaara Awaiting requested review from MohamadJaara

@vitorhugods vitorhugods Awaiting requested review from vitorhugods

@ohassine ohassine Awaiting requested review from ohassine

Assignees

@borichellow borichellow

Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@borichellow @AndroidBob @mchenani @saleniuk