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

Update shields to match web #4338

Merged
merged 4 commits into from
Nov 17, 2021
Merged

Update shields to match web #4338

merged 4 commits into from
Nov 17, 2021

Conversation

BillCarsonFr
Copy link
Member

Updated shield drawable to match those on wed (see https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=1373%3A13)
Fixes element-hq/element-meta#52

There is still a difference though, the icons have a white border, but looks like on web they don't (see verification conclusion in dark theme on web)

image

@BillCarsonFr BillCarsonFr added the X-Needs-Design May require input from the design team label Oct 26, 2021
@github-actions
Copy link

github-actions bot commented Oct 26, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   1m 4s ⏱️ +12s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 78bdef4. ± Comparison against base commit a8f6efd.

♻️ This comment has been updated with latest results.

@bmarty
Copy link
Member

bmarty commented Oct 26, 2021

Can you add a file for the changelog please?

@BillCarsonFr
Copy link
Member Author

Can you add a file for the changelog please?

Yes, I am waiting for some inputs (potential copy change), and also why web removes the white border on dark mode (a bit annoying cause we are reusing the same component for shields on android)

@BillCarsonFr BillCarsonFr force-pushed the feature/bca/fix_crypto_shields branch from e15e07a to 11747fb Compare November 17, 2021 08:26
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Just a small remark on the impl, else LGTM

@@ -35,10 +35,23 @@ abstract class BottomSheetVerificationBigImageItem : VectorEpoxyModel<BottomShee

override fun bind(holder: Holder) {
super.bind(holder)
holder.image.render(roomEncryptionTrustLevel)
when (roomEncryptionTrustLevel) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep the old code and add a optional parameter in the ShieldImageView fun render(..., noBorder: Boolean = false) to use alternative image in the case the parameter is true.
Can you do that please?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

<im.vector.app.core.ui.views.ShieldImageView xmlns:android="http://schemas.android.com/apk/res/android"
<ImageView xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:contentDescription="@string/trusted"
Copy link
Member

Choose a reason for hiding this comment

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

android:contentDescription -> tools:contentDescription

Also please format the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -1,5 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<im.vector.app.core.ui.views.ShieldImageView xmlns:android="http://schemas.android.com/apk/res/android"
<ImageView xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Member

Choose a reason for hiding this comment

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

So you can revert the change on this line too

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@BillCarsonFr BillCarsonFr force-pushed the feature/bca/fix_crypto_shields branch from e861314 to 78bdef4 Compare November 17, 2021 15:39
@@ -35,7 +35,7 @@ abstract class BottomSheetVerificationBigImageItem : VectorEpoxyModel<BottomShee

override fun bind(holder: Holder) {
super.bind(holder)
holder.image.render(roomEncryptionTrustLevel)
holder.image.render(roomEncryptionTrustLevel, borderLess = true)
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, I've double check all the vector drawables, and everything is fine

@bmarty bmarty merged commit 0c4dee8 into develop Nov 17, 2021
@bmarty bmarty deleted the feature/bca/fix_crypto_shields branch November 17, 2021 16:43
@chagai95
Copy link
Contributor

Is there a plan to have this as a config?
Unfortunately, we have another color theme...

isVisible = roomEncryptionTrustLevel != null

when (roomEncryptionTrustLevel) {
RoomEncryptionTrustLevel.Default -> {
contentDescription = context.getString(R.string.a11y_trust_level_default)
setImageResource(R.drawable.ic_shield_black)
setImageResource(
Copy link
Contributor

Choose a reason for hiding this comment

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

@chagai95 updating these to use setAttributeTintedImageResource and then supplying a theme colour will allow customisations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-Needs-Design May require input from the design team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the shields the same in Element Web and Element Android
4 participants