Skip to content

Threat Details: Skeleton with blank Content UiState#13675

Merged
ParaskP7 merged 20 commits intodevelopfrom
issue/13326-threat-details-skeleton-and-blank-content-uistate
Jan 5, 2021
Merged

Threat Details: Skeleton with blank Content UiState#13675
ParaskP7 merged 20 commits intodevelopfrom
issue/13326-threat-details-skeleton-and-blank-content-uistate

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Dec 24, 2020 •
edited
Loading

Parent: #13326

Description

This PR does the following for "Threat Details"

To test

Prerequisite: Make sure both Scan feature flag and MySiteImprovements flag are set to on in the App settings.

  1. Select a site with scan capability (and having threats) on the My Site tab
  2. Notice that scan menu is displayed
  3. Click scan menu item
  4. Notice that threats are shown
  5. Click a threat item
  6. Notice that blank Threat Details screen is displayed

Merge Instructions

  1. Make sure FluxC PR ScanStore: Get threat details by id WordPress-FluxC-Android#1819 is merged and tag is created in develop - Merged
  2. Update FluxC hash with tag (created in FluxC develop) - Done in 4f4b947
  3. Remove the "Not Ready for Merge label" - Done
  4. Merge this PR

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.5 milestone Dec 24, 2020
@ashiagr ashiagr self-assigned this Dec 24, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 24, 2020 •
edited
Loading

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

@ashiagr ashiagr mentioned this pull request Dec 24, 2020
Closed
67 tasks
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 24, 2020 •
edited
Loading

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

@ashiagr ashiagr marked this pull request as ready for review January 4, 2021 04:57
@ashiagr ashiagr requested a review from ParaskP7 January 4, 2021 04:58
This was referenced Jan 4, 2021
Merged
Merged
@ParaskP7 ParaskP7 self-assigned this Jan 4, 2021
ashiagr
ashiagr commented Jan 4, 2021
View reviewed changes
...src/main/java/org/wordpress/android/ui/jetpack/scan/details/adapters/ThreatDetailsAdapter.kt
Comment on lines +26 to +27
ViewType.ICON.id -> JetpackIconViewHolder(imageManager, parent)
ViewType.DESCRIPTION.id -> JetpackDescriptionViewHolder(uiHelpers, parent)
Copy link
Copy Markdown
Contributor Author

@ashiagr ashiagr Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

These are placeholder ViewTypes, appropriate ones are added in a separate PR #13703.

ParaskP7
ParaskP7 approved these changes Jan 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

Great job @ashiagr ! 🌟

I have tested the solution (as per the instructions) and can verify correctness.

I have left few minors, questions and ideas here and there but overall I am approving the PR since those are not blocking. Feel free to merge the PR anyway. I am not merging the PR myself since I want to give you the opportunity to first check my comments.

PS: I really love the way you split your commits, kudos (❤️)! It really helped me review the PR and understand the way the solution was built. Also, I loved the fact that you added the Refactor: action prefix on your initial commits. Actually, my team used to do that in the past as well and we found out that it works miracles. I wonder if you folks would like to start using this just as well, for a trial period of time and see if you like it. Your thoughts @zwarm @malinajirka ?

@ashiagr ashiagr requested a review from ParaskP7 January 5, 2021 05:19
@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 5, 2021

Thanks @ParaskP7! I've addressed your comments and answered questions. I've also updated fluxc tag from develop and removed "Not Ready for Merge" label.

Ready for another look 🙏.

@ParaskP7
Copy link
Copy Markdown
Contributor

ParaskP7 commented Jan 5, 2021

Thanks @ParaskP7! I've addressed your comments and answered questions. I've also updated fluxc tag from develop and removed "Not Ready for Merge" label.

Thanks for all the changes @ashiagr! 🌟

It looks good, I am re-approving and will be merging this PR now.

@ParaskP7 ParaskP7 merged commit f83f4c2 into develop Jan 5, 2021
@ParaskP7 ParaskP7 deleted the issue/13326-threat-details-skeleton-and-blank-content-uistate branch January 5, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ParaskP7 ParaskP7 ParaskP7 approved these changes

Assignees

@ashiagr ashiagr

@ParaskP7 ParaskP7

Labels

Jetpack

Projects

None yet

Milestone

16.5

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @ParaskP7