Skip to content

Manage Insights screen#9501

Merged
planarvoid merged 74 commits intofeature/manage_insights_readonlyfrom
issue/9464-manage-insights
Apr 23, 2019
Merged

Manage Insights screen#9501
planarvoid merged 74 commits intofeature/manage_insights_readonlyfrom
issue/9464-manage-insights

Conversation

@0nko
Copy link
Copy Markdown
Contributor

@0nko 0nko commented Apr 1, 2019

Fixes #9464.

To test:

  1. Go to Stats
  2. Tap the Manage toolbar button
  3. Try adding and removing the insights by tapping on the + or - button
  4. Tap the Save button
  5. Notice the Insights list is updated and data is refreshed
  6. Go back to the management screen
  7. Try reordering the added insights by drag & dropping of list items
  8. Tap the Save button
  9. Notice the Insights list is reordered accordingly

image

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

0nko added 29 commits March 25, 2019 21:06
@0nko
Add insights management list item resources
3f6e429
@0nko
Update the insights management fragment layout with text items
ba8fcc9
@0nko
Add the insights management list adapter and view holder
aaa5567
@0nko
Add InsightModel and implement the removed insights loading
cc5c80a
@0nko
Add the drawable resources for insights management
fae598f
@0nko
Add new drag & drop hint string resource
32136a4
@0nko
Update the list item layout
b54109f
@0nko
Add the added insights list and drag & drop hint
b4f3b2e
@0nko
Add drag & drop ordering to added insights
20665e3
@0nko
Remove redundant code
ed85544
@0nko
Add the save button to the insights management screen
92d2c0b
@0nko
Fix snackbar display
46fae9b
@0nko
Update the ViewModel after dragging items
ed38594
@0nko
Whitespace clean up
a6a2b8a
@0nko
Fix insights diff util comparisons
2671904
@0nko
Add insights adding and removal functionality
9fae18a
@0nko
Update string resources
36b8aa2
@0nko
Create separate for added and removed insight types
00bed85
@0nko
Animate list resize changes
2634449
@0nko
Merge branch 'feature/manage_insights_readonly' into issue/9464-manag…
47d7c1a 
…e-insights
@0nko
Close the insights management instead of showing a snackbar message
9e33dc9
@0nko
Update the FluxC reference
39a2f9d
@0nko
Rename the layout view IDs for consistency
f9830d8
@0nko
Initialize empty and error views
d53c388
@0nko
Remove unused string resource
08b47ad
@0nko
Add list item decorators
fa838f8
@0nko
Don't wait for data to refresh when leaving the insights management
08973d1
@0nko
Fix the empty isnights actionable screen display
24f5351
@0nko
Fix ktlint
8e6998b
planarvoid
planarvoid reviewed Apr 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Check my comments about the LinkButton block and let me know what you think

WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListViewModel.kt Outdated
popupMenuHandler
)
) {
override val uiModel: LiveData<UiModel> = super.uiModel.map { model ->
Copy link
Copy Markdown
Contributor

@planarvoid planarvoid Apr 15, 2019

Choose a reason for hiding this comment

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

I think this belongs to the org.wordpress.android.ui.stats.refresh.lists.UiModelMapper#mapInsights method cause that's where we build the UiModel. You can either pass the mutableNavigationTarget as a parameter to the mapInsights function. What do you think?

Copy link
Copy Markdown
Contributor Author

@0nko 0nko Apr 15, 2019

Choose a reason for hiding this comment

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

Good point!

WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListViewModel.kt Outdated
}
}

enum class InsightsTypes : StatsTypes {
Copy link
Copy Markdown
Contributor

@planarvoid planarvoid Apr 15, 2019

Choose a reason for hiding this comment

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

I'd remove the StatsTypes from the base StatsBlock implementation. I think the Control block doesn't need type. We just need to do a bit of casting in the StatsBlockAdapter and modify the DiffUtils so they don't check the statsTypes when it's comparing the Control block

@0nko 0nko requested a review from planarvoid April 16, 2019 10:07
@planarvoid
Copy link
Copy Markdown
Contributor

planarvoid commented Apr 16, 2019

Everything looks pretty great!

One thing is that adding a new block still keeps it in a permanent loading state (the first time you add it). Do you want to tackle this in a separate PR?

@SylvesterWilmott
Copy link
Copy Markdown

SylvesterWilmott commented Apr 17, 2019

Thanks @0nko

Dragging items in the list feels so good now, it's looking really solid, few things:

  • Manage view: Let's reuse the maximum width from the detail views for landscape/tablet
  • Insights view: Is there a reason we're not using the system text button for "Edit" at the bottom of the list? If yes then can we recreate the styling of a system button ie. Medium text weight, 36dp height, 20dp left and right padding
  • Manage view: There's an odd transitional movement within the manage insights list whenever you open the view, it looks as though the "Add insights" section is moving downwards.
  • Manage view: Let's add an additional 16dp of space between the top section footer and the next section:

Screenshot 2019-04-17 at 14 44 53

Regarding the functionality, there still seems to be one issue:

  1. Visit stats view of a site that has insights set fo default layout
  2. Use the block menu (from the insights view) to move any card up/down
  3. Go to the manage insights view
  4. Notice that "Add insights" section is missing and cannot be restored even by killing the app

0nko added 14 commits April 17, 2019 19:27
@0nko
Fix the newly added insights data loading
8645831
@0nko
Add additional padding between the footer and the Add Insights header
ddbf15a
@0nko
Add the reduced width layout for wide screens (landcape)
02bf4e7
@0nko
Add a custom divider that has a padding at the beginning
35bbc91
@0nko
Use a regular button style
b0b080f
@0nko
Delay animations until the insights are loaded to prevent weird slide…
574ded8 
… in effect
@0nko
Update the layout paddings
bbbdccc
@0nko
Merge branch 'feature/manage_insights_readonly' into issue/9464-manag…
afd2221 
…e-insights

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/StatsFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/StatsViewModel.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/BaseListUseCase.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListViewModel.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/utils/ItemPopupMenuHandler.kt
@0nko
Update the FluxC reference and make the necessary code changes
11a9497
@0nko
Fix the doble blocks bug
a5c3ac9
@0nko
Always show the Edit button
e1ab010
@0nko
Correct the layout paddings
f30ac7f
@0nko
Fix the failing unit test
7874a42
@0nko
Add the context menu to the Annual stats block
4640b84
@SylvesterWilmott
Copy link
Copy Markdown

SylvesterWilmott commented Apr 23, 2019

This looks and feels amazing @0nko , thanks!

@SylvesterWilmott SylvesterWilmott removed the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Apr 23, 2019
planarvoid
planarvoid approved these changes Apr 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

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

Looks good 👍 , as discussed, we'll move the fix for the infinitely loading blocks to a separate PR/issue

@planarvoid planarvoid merged commit 96de67f into feature/manage_insights_readonly Apr 23, 2019
@planarvoid planarvoid deleted the issue/9464-manage-insights branch April 23, 2019 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@planarvoid planarvoid planarvoid approved these changes

Assignees

@planarvoid planarvoid

@0nko 0nko

Labels

Stats

Projects

None yet

Milestone

12.3

Development

Successfully merging this pull request may close these issues.

4 participants

@0nko @planarvoid @SylvesterWilmott @irfano