Skip to content

Issue/13326 scan screen UI iteration 1#13873

Merged
malinajirka merged 13 commits intodevelopfrom
issue/13326-scan-screen-ui-iteration-1
Jan 25, 2021
Merged

Issue/13326 scan screen UI iteration 1#13873
malinajirka merged 13 commits intodevelopfrom
issue/13326-scan-screen-ui-iteration-1

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 25, 2021 •
edited
Loading

Parent #13326

This PR makes small UI improvements to the Scan screen for scan state items:

  • Separates primary and secondary buttons
  • Add icon margin, size dynamically (for different margins, sizes on scan and threat details screen)
  • Adds vertical and horizontal margins to the scan screen items

To test:

That scan states UI is rendered properly:

scan ok scanning threats found fixing
scan_okay scanning threats_found fixing

Notes:

  1. Review by commits might be easier
  2. Yet to add CTA for "here to help" - added to backlog

Merge Instructions

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ashiagr ashiagr added this to the 16.6 milestone Jan 25, 2021
@ashiagr ashiagr self-assigned this Jan 25, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 25, 2021 •
edited
Loading

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

@ashiagr ashiagr requested a review from malinajirka January 25, 2021 08:11
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 25, 2021 •
edited
Loading

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

@malinajirka malinajirka self-assigned this Jan 25, 2021
malinajirka
malinajirka reviewed Jan 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @ashiagr, the changes look good overall. I noticed one minor issue - the icon on the "scanning" doesn't match the designs.

SideNote: I'm still not sure about the JetpackIconViewHolder - I'm wondering if re-using this part is worth it. We might want to consider having a Header for scan screen and a different header for detail screen. Wdyt?

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 25, 2021

Thanks for the review @malinajirka!

SideNote: I'm still not sure about the JetpackIconViewHolder - I'm wondering if re-using this part is worth it. We might want to consider having a Header for scan screen and a different header for detail screen. Wdyt?

This view holder is reused on other screens too: backup, restore etc. I can discuss with @zwarm and decide upon it.

the icon on the "scanning" doesn't match the designs.

I'll look into it, seem like dynamic tinting is causing some issues.

Base automatically changed from issue/13326-fix-threats-status-progress-labels to develop January 25, 2021 11:02
@ashiagr ashiagr modified the milestones: 16.6, 16.7 Jan 25, 2021
@ashiagr ashiagr mentioned this pull request Jan 25, 2021
Closed
67 tasks
@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 25, 2021

the icon on the "scanning" doesn't match the designs.

@malinajirka I couldn't find a solution to this problem yet. I've replaced the icon with the one without stroke and added this issue to the backlog.

Screenshot 2021-01-25 at 6 50 14 PM

Ready for another round!

malinajirka
malinajirka approved these changes Jan 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka 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!

@malinajirka malinajirka merged commit abb84de into develop Jan 25, 2021
@malinajirka malinajirka deleted the issue/13326-scan-screen-ui-iteration-1 branch January 25, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka approved these changes

Assignees

@ashiagr ashiagr

@malinajirka malinajirka

Labels

None yet

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @malinajirka