You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

You can test the changes on this Pull Request by downloading the APK here.

👋 Hey @ashiagr thanks for sending the PR. It's great to see everything coming together. Here are some tweaks and details, but none are blockers.

• In the subheading "The scan found 4 potential threats..." is that the default line spacing? The space between the lines looks tight.
• the heading 'threats found' looks to be a bold/medium font weight. This was just to be a section header (e.g. in list view on My Site). In other places we use that section header the font weight is regular/normal rather than bold
• the subtitle in the row looks like very small text. In design I had 14dp - looks like it might be 12dp. But maybe that is standard? The same is also true on Scan History (per issue Issue/13326 threat details & history design review #13950) , the subtitle on those rows looks small
• Preparing to scan: might need different icon here. Let's discuss together how to get the right one in the app.
• In 'fixing threat' state I think you may have adopted the approach suggested by Emily to show loading state on the threat being removed? I didn't know we had committed to that. But if possible, I think it could be good to have. Could the text sizes here be the same as other rows (e.g. 'threats found' list items). It looks a bit smaller. Also I think we should add another 16dp space between the loading bar and this row
• Is the progress bars (fixing threats, scanning) one that we already use elsewhere in the app? I would tweak it, but if its a standard component we use everywhere I'll leave it alone!
• Error states - are they all re-using existing ui and type sizes?

Scanning progress bar positioning - it wasn't included in the last few mockups but I see it in Zeplin now. I can move it above the description but not sure if we should display progress values (we get them from the API).

I'm not worried about this one. Its fine with the positioning that you have.

Thanks so much for the review @osullivanchris!

• the subtitle in the row looks like very small text. In design I had 14dp - looks like it might be 12dp. But maybe that is standard? The same is also true on Scan History (per issue Issue/13326 threat details & history design review #13950) , the subtitle on those rows looks small

Threat item sub header uses the same text appearance as the Activity Log event item: textAppearanceCaption. Should we change to textAppearanceBody2 for both?

• the heading 'threats found' looks to be a bold/medium font weight. This was just to be a section header (e.g. in list view on My Site). In other places we use that section header the font weight is regular/normal rather than bold

Fixed in f784a1c

Activity Log Event	Threats Found

Is the progress bars (fixing threats, scanning) one that we already use elsewhere in the app? I would tweak it, but if its a standard component we use everywhere I'll leave it alone!

Yes its the same as used on other screens (backup, restore).

Error states - are they all re-using existing ui and type sizes?

Yes, they re-use existing error ui and type sizes. Same style is used on Scan History screen too. Not sure if we'd like to update style on them.

Let's sync to discuss the remaining:

• In the subheading "The scan found 4 potential threats..." is that the default line spacing? The space between the lines looks tight.
• Preparing to scan: might need different icon here. Let's discuss together how to get the right one in the app.
• In 'fixing threat' state I think you may have adopted the approach suggested by Emily to show loading state on the threat being removed? I didn't know we had committed to that. But if possible, I think it could be good to have. Could the text sizes here be the same as other rows (e.g. 'threats found' list items). It looks a bit smaller. Also I think we should add another 16dp space between the loading bar and this row

Threat item sub header uses the same text appearance as the Activity Log event item: textAppearanceCaption. Should we change to textAppearanceBody2 for both?

Let's leave it for the time being then. If its a style we're using globally I don't want to fix it. Sounds like that applies to a few of the other issues as well.

Let's sync to discuss the remaining:

👍

Thanks for our zoom call @osullivanchris!

As discussed,

In the subheading "The scan found 4 potential threats..." is that the default line spacing? The space between the lines looks tight.

Increased line height to 1.3

Preparing to scan: might need different icon here. Let's discuss together how to get the right one in the app.

We're using the right color

We can't yet add a stroke/ border to the scanning icon due to a dynamic tinting issue (it's a known issue added to feature backlog).

In 'fixing threat' state I think you may have adopted the approach suggested by Emily to show loading state on the threat being removed? I didn't know we had committed to that. But if possible, I think it could be good to have. Could the text sizes here be the same as other rows (e.g. 'threats found' list items). It looks a bit smaller. Also I think we should add another 16dp space between the loading bar and this row

We're keeping similar approach for threat fixing on iOS and Android for feature similarity. UI might need some tweaking but not in this version.
Threat item row is re-used between threats found and fixing threats states, so they share the same text styles.
Added "Threats found" header below the progress bar which accounts for some spacing below it.

Ready for another look 🙇‍♀️.

Taking a look at the build again now.

Here are colour values for the code blocks as discussed. Tried to use High Emphasis (.87) for everything but none of the greys were working for me at that value so I used medium (.6) for a couple.

@zwarm I added you as a reviewer as we share styles between our features.

This might be of interest to you (approved by Chris for new jetpack screens):
Increase line spacing for description 33d9aff

Increased line height to 1.3

Looks good! And checked in restore/backup too. Happy with this.

We're using the right color

We can't yet add a stroke/ border to the scanning icon due to a dynamic tinting issue (it's a known issue added to feature backlog).

Can revisit another time. 👍 for now.

Threat item row is re-used between threats found and fixing threats states, so they share the same text styles.

👍

Added "Threats found" header below the progress bar which accounts for some spacing below it.

Looks much better, thanks.

All good from my side here I think.

I'll address #13949 (comment) in Threat Details Design Review PR #13950.

@zwarm, need your approval to merge it.