Skip to content

Jetpack Section: Scan: Adds Scan History View#15696

Merged
emilylaguna merged 18 commits intodevelopfrom
task/15190-scan-history
Jan 28, 2021
Merged

Jetpack Section: Scan: Adds Scan History View#15696
emilylaguna merged 18 commits intodevelopfrom
task/15190-scan-history

Conversation

@emilylaguna
Copy link
Copy Markdown
Contributor

@emilylaguna emilylaguna commented Jan 22, 2021 •
edited
Loading

Project: #15190
Related PR: wordpress-mobile/WordPressKit-iOS#331

Adds scan history with error handling.

The error messages aren't solidified yet and will need copy from @osullivanchris

Screenshots

General States

Loading All Fixed Ignored
loading all fixed ignored

Empty States

No History No Fixed No Ignored
nohistory nofixed noignored

Error States

Generic Error No Internet
generic nointernet

To test:

  1. Launch the app, tap on the My Site, tap on a site with Jetpack Scan
  2. Tap the history item in the top right corner
  3. 👁️ You should see the scan history view with a loading indicator
  4. 👁️ Once it finishes loading you'll see a list of threats
  5. Tap the 'Fixed' tab item
  6. 👁️ You'll only see a list of fixed threats (green color)
  7. Tap the 'Ignored' tab item
  8. 👁️ You'll only see a list of ignored threats (gray color)
  9. Pull to refresh and wait for it to complete
  10. 👁️ List is auto filtered using the tab item you had selected before

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.

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

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

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 22, 2021

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

This was referenced Jan 22, 2021
Closed
Merged
@peril-wordpress-mobile
Copy link
Copy Markdown

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

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

momo-ozawa
momo-ozawa reviewed Jan 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

The site I'm testing has fixed threats but no ignored threats. It seems like section titles from the Fixed tab are still visible in the Ignored tab. If you scroll on the Ignored tab, you can see the cells from the Fixed tab scrolling as well.

Fixed Ignored
Simulator Screen Shot - iPhone 12 Pro - 2021-01-25 at 10 56 06 Simulator Screen Shot - iPhone 12 Pro - 2021-01-25 at 10 56 09

@jkmassel
Copy link
Copy Markdown
Contributor

jkmassel commented Jan 25, 2021

👋 Howdy! We are cutting the 16.6 release today.

Because of that, this PR will be bumped to 16.7. If you need it to be part of 16.6, please merge it into the release/16.6 branch and DM me – I'll be happy to cut a new beta release!

@jkmassel jkmassel modified the milestones: 16.6, 16.7 Jan 25, 2021
@emilylaguna
Copy link
Copy Markdown
Contributor Author

emilylaguna commented Jan 25, 2021

Ah good catch! Fixed in: 1cdbb5d

momo-ozawa
momo-ozawa reviewed Jan 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa 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 for fixing the sections for empty views! Another thing I noticed about the empty views: when you swipe to refresh, the refresh control isn't displayed.

Otherwise, works as described 👍

momo-ozawa
momo-ozawa reviewed Jan 26, 2021
View reviewed changes
WordPress/Classes/ViewRelated/Jetpack/Jetpack Scan/JetpackScanViewController.swift Outdated
configureTableView()
coordinator.viewDidLoad()

navigationItem.rightBarButtonItem = UIBarButtonItem(title: "History",
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa Jan 26, 2021

Choose a reason for hiding this comment

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

👀 "History" button should be localized

Copy link
Copy Markdown
Contributor Author

@emilylaguna emilylaguna Jan 27, 2021

Choose a reason for hiding this comment

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

NooOooO!!!! Haha, done in e42ca2c

momo-ozawa
momo-ozawa approved these changes Jan 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Another thing I noticed about the empty views: when you swipe to refresh, the refresh control isn't displayed.

I think this ^ can be addressed in a future PR, so I'm gonna go ahead and approve this PR 😄

@emilylaguna emilylaguna merged commit 9001faf into develop Jan 28, 2021
@emilylaguna emilylaguna deleted the task/15190-scan-history branch January 28, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandroalonso leandroalonso Awaiting requested review from leandroalonso

1 more reviewer

@momo-ozawa momo-ozawa momo-ozawa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@emilylaguna emilylaguna

Labels

Jetpack Mobile [Type] Task

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

3 participants

@emilylaguna @jkmassel @momo-ozawa