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

Search context menus #705

Merged
merged 2 commits into from
Nov 11, 2023
Merged

Search context menus #705

merged 2 commits into from
Nov 11, 2023

Conversation

Sjmarf
Copy link
Contributor

@Sjmarf Sjmarf commented Oct 7, 2023

Lets you share and block communities and users from a context menu on each search result.
IMG_9555

@Sjmarf Sjmarf requested a review from a team as a code owner October 7, 2023 11:14
@Sjmarf Sjmarf requested review from JakeShirley and EricBAndrews and removed request for a team October 7, 2023 11:14
Copy link
Contributor

@boscojwho boscojwho 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, works as expected:

  • Would it be possible to move the destructive actions (block/unsubscribe) into its own section in the menu? Always makes me a little nervous having a destructive menu item as the first option on a long press menu.
  • Not PR block worthy though, if too big of a change for this PR to how menu functions/items are generated.

@Sjmarf
Copy link
Contributor Author

Sjmarf commented Oct 20, 2023

Would it be possible to move the destructive actions (block/unsubscribe) into its own section in the menu?

When you say 'section', do you mean adding a divider or adding a separate sub-menu like we use for the 'top' sort modes in the feed?

I don't think misclicks should be a huge issue - the app asks for confirmation for both unsubscribing and blocking anyways

Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Sjmarf Sjmarf force-pushed the sjmarf/search-context-menus branch from 37c82e9 to 7623d51 Compare November 10, 2023 13:56
@Sjmarf
Copy link
Contributor Author

Sjmarf commented Nov 10, 2023

I've had to rewrite most of this - it was quite old haha. I've added some extra changes that weren't present in the previous version:

  • The subscribe swipe action now has a haptic (like the post swipe actions)
  • Unsubscribing from a community via the swipe action now shows the 'are you sure?' popup
  • Doubled the thumbnail resolution in the search tab (64 -> 128)

@boscojwho
Copy link
Contributor

Looks good:

  • Actions work via swipe or context menu, as expected.
  • Drag and drop works (tested by dragging link into Safari).
  • Share menu generates expected items.
  • Tested on iOS 17 simulator (so couldn't test swipe haptics).

@Sjmarf Sjmarf merged commit 3a2a7c0 into dev Nov 11, 2023
4 checks passed
@Sjmarf Sjmarf deleted the sjmarf/search-context-menus branch November 11, 2023 20:16
boscojwho pushed a commit that referenced this pull request Nov 23, 2023
Co-authored-by: Bosco Ho <[email protected]>
(cherry picked from commit 3a2a7c0)
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.

3 participants