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

Add temporary icon for Element Enterprise #3134

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Add temporary icon for Element Enterprise #3134

merged 8 commits into from
Jul 4, 2024

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Jul 3, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add temporary icon to Element Enterprise, currently it is using the same icon than Element X, which can be confusing

Motivation and context

Screenshots / GIFs

image

Tests

  • Run Element X: no change
  • Build and run EE: new icon with "PRO" mention.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

  • Changes have been tested on an Android device or Android emulator with API 23
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off
  • You've made a self review of your PR

@bmarty bmarty requested a review from a team as a code owner July 3, 2024 15:45
@bmarty bmarty requested review from jmartinesp and removed request for a team July 3, 2024 15:45
@bmarty bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 3, 2024
@github-actions github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 3, 2024
@bmarty bmarty added the PR-Misc For other changes label Jul 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024 •
edited
Loading

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/iHXVrG

bmarty
bmarty commented Jul 3, 2024
View reviewed changes
appicon/enterprise/src/main/res/mipmap-anydpi/ic_launcher.xml
Copy link
Member Author

Choose a reason for hiding this comment

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

Not: no need to use -v26 qualifier and have webp icons, since EE is API 26 min 🎉

bmarty
bmarty commented Jul 3, 2024
View reviewed changes
tests/uitests/src/test/snapshots/images/appicon.element_RoundIcon_en.png
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like since the uitest module is importing the 2 icon modules, and the resource for foreground is the same, there is some clash here. Will fix.

@bmarty bmarty force-pushed the feature/bma/applicationIcons branch from 50598ea to c823ca9 Compare July 3, 2024 16:02
@bmarty bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 3, 2024
@github-actions github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 3, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.02%. Comparing base (3fb0c06) to head (7c63c5f).
Report is 5 commits behind head on develop.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #3134   +/-   ##
========================================
  Coverage    76.01%   76.02%           
========================================
  Files         1635     1633    -2     
  Lines        38578    38572    -6     
  Branches      7460     7460           
========================================
- Hits         29326    29325    -1     
+ Misses        5361     5355    -6     
- Partials      3891     3892    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@bmarty bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jul 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jul 3, 2024
jmartinesp
jmartinesp approved these changes Jul 4, 2024
View reviewed changes
Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Having a separate module just for the app icon sound a bit like an overkill, but I see why it's needed.

gradle/libs.versions.toml
@@ -145,6 +145,7 @@ test_truth = "com.google.truth:truth:1.4.3"
test_parameter_injector = "com.google.testparameterinjector:test-parameter-injector:1.16"
test_robolectric = "org.robolectric:robolectric:4.12.2"
test_appyx_junit = { module = "com.bumble.appyx:testing-junit4", version.ref = "appyx" }
test_composable_preview_scanner = "com.github.sergio-sastre.ComposablePreviewScanner:android:0.1.2"
Copy link
Member

Choose a reason for hiding this comment

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

Oops, thanks!

@bmarty bmarty merged commit 646791e into develop Jul 4, 2024
30 checks passed
@bmarty bmarty deleted the feature/bma/applicationIcons branch July 4, 2024 09:57
@bmarty
Copy link
Member Author

bmarty commented Jul 4, 2024

LGTM, thanks. Having a separate module just for the app icon sound a bit like an overkill, but I see why it's needed.

Yeah, I did not find any other solution. Overriding the value for icons in a submodule AndroidManifest.xml cannot work, value from a lower priority manifest cannot replace a value in the highest priority manifest.

The good side of things is that now the preview of the icon can be recorded.

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

@jmartinesp jmartinesp jmartinesp approved these changes

Assignees
No one assigned
Labels
PR-Misc For other changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bmarty @jmartinesp