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

Mise à jour des icônes de l'application #31

Merged
merged 3 commits into from
Jan 28, 2025
Merged

Mise à jour des icônes de l'application #31

merged 3 commits into from
Jan 28, 2025

Conversation

yostyle
Copy link

@yostyle yostyle commented Jan 27, 2025 •
edited
Loading

Content

Motivation and context

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

  • Changes have been tested on an Android device or Android emulator with API 24
  • 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
  • You've made a self review of your PR

@yostyle yostyle requested a review from NicolasBuquet January 27, 2025 18:08
@yostyle yostyle self-assigned this Jan 27, 2025
@yostyle yostyle added the PR-Change For updates to an existing feature label Jan 27, 2025
NicolasBuquet
NicolasBuquet reviewed Jan 28, 2025
View reviewed changes
Copy link

@NicolasBuquet NicolasBuquet left a comment

Choose a reason for hiding this comment

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

2 remarques qui m'interrogent.

appicon/tchap/src/main/kotlin/fr/gouv/tchap/android/appicon/tchap/IconPreview.kt
Copy link

Choose a reason for hiding this comment

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

Surprenant que le code référence 2x la même ressource : R.drawable.ic_launcher_monochrome

Copy link
Author

Choose a reason for hiding this comment

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

c'est une erreur de ma part. les previews sont HS

appicon/tchap/src/release/res/drawable/ic_launcher_foreground.xml
Copy link

Choose a reason for hiding this comment

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

Surprenant que les viewportWidth et viewportHeight ne soient pas égales pour une icone carrée et que ces valeurs ne soient pas les mêmes pour tous les xml.

Copy link
Author

@yostyle yostyle Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

il s'agit de la partie foreground de l'icône, donc la partie hexagonale (zone blanche). la hauteur est supérieure à la largeur avec la nouvelle version

@yostyle yostyle added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 28, 2025
@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 Jan 28, 2025
NicolasBuquet
NicolasBuquet approved these changes Jan 28, 2025
View reviewed changes
Copy link

@NicolasBuquet NicolasBuquet left a comment

Choose a reason for hiding this comment

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

LGTM

@yostyle yostyle changed the title Add Tchap X icons Mise à jour des icônes de l'application Jan 28, 2025
@yostyle yostyle merged commit d3e2d0a into develop Jan 28, 2025
20 of 21 checks passed
@yostyle yostyle deleted the yostyle/icon branch January 28, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasBuquet NicolasBuquet NicolasBuquet approved these changes

Assignees

@yostyle yostyle

Labels
PR-Change For updates to an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yostyle @NicolasBuquet