Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/14004 menu item order #14183

Merged
merged 17 commits into from
Mar 4, 2021
Merged

Issue/14004 menu item order #14183

merged 17 commits into from
Mar 4, 2021

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Mar 3, 2021

Fixes #14004

This PR changes the More menu on the ReaderPost card

  • The menu item order has changed
  • The block site/report this post test/icon have changed color to red
  • There is an empty row between share and the block/report actions
current light dark
current light dark

To test:
Scenario One

  • Launch the app
  • Navigate to discover
  • Tap the more menu on a card.
  • Note that following:
    -- The item order has changed
    -- There is a space before the block this site and/or report this site

Scenario Two

  • Launch the app

  • Navigate to following

  • Tap the more menu on a card.

  • Note that following:
    -- The item order has changed
    -- There is a space before the block this site and/or report this site
    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.

@zwarm zwarm added this to the 16.9 milestone Mar 3, 2021
@zwarm zwarm requested a review from ParaskP7 March 3, 2021 22:54
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 3, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 3, 2021

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

@ParaskP7 ParaskP7 self-assigned this Mar 4, 2021
Copy link
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.

👋 @zwarm !

I have reviewed and tested this PR as per the instructions, everything works as expected, great work here! 🌟 🌟

I have left minor (🔍) comments, suggestions (💡) and some minor warnings (⚠️) for you to consider.

Tip (ℹ️): Whenever you improve with Detekt, fixing an issue or suppress one, make sure to run the ./gradlew WordPress:detektBaseline command and see if that removes or updates an issue in the baseline. For example, your work on the LongMethod:ReaderPostMoreButtonUiStateBuilder.buildMoreMenuItemsBlocking(...) function, actually cleaned up the baseline by that one line! ❤️

<ID>LongMethod:ReaderPostMoreButtonUiStateBuilder.kt$ReaderPostMoreButtonUiStateBuilder$fun buildMoreMenuItemsBlocking( post: ReaderPost, onButtonClicked: (Long, Long, ReaderPostCardActionType) -&gt; Unit ): MutableList&lt;SecondaryAction&gt;</ID>

@zwarm
Copy link
Contributor Author

zwarm commented Mar 4, 2021

@ParaskP7 - I incorporated all your suggestions. Thanks!

@zwarm zwarm requested a review from ParaskP7 March 4, 2021 13:10
Copy link
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.

👋 @zwarm !

@ParaskP7 - I incorporated all your suggestions. Thanks!

I checked all the new changes, thank you so much for all that, LGTM! 🌟

@ParaskP7 ParaskP7 merged commit 667e2ca into develop Mar 4, 2021
@ParaskP7 ParaskP7 deleted the issue/14004-menu-item-order branch March 4, 2021 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change order of more menu items so "Visit site" won't be next to "Block site"
2 participants