Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Ref #1499: Add link preview to long press context menu. #1625

Merged
merged 4 commits into from
Oct 14, 2019

Conversation

iccub
Copy link
Contributor

@iccub iccub commented Oct 10, 2019

Summary of Changes

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Zrzut ekranu 2019-10-10 o 18 56 27

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

@iccub iccub changed the title Ref #1499: Add link preview to long press context. Ref #1499: Add link preview to long press context menu. Oct 10, 2019
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Also, Safari provides a way to disable these previews, we may want to give the same option to users

@@ -2511,11 +2511,21 @@ extension BrowserViewController: WKUIDelegate {
return UIMenu(title: url.absoluteString, children: actions)
}

let config = UIContextMenuConfiguration(identifier: nil, previewProvider: nil, actionProvider: actionProvider)
let linkPreview: UIContextMenuContentPreviewProvider = {
return SFSafariViewController(url: url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to make our own preview provider that uses BVC in some way (so shields are loaded in and such)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UIContextMenuContentPreviewProvider accepts any UIViewController so we could try to add some custom view with content blocker lists

@kylehickinson
Copy link
Collaborator

Sorry that review meant to be comment, not request changes

@iccub
Copy link
Contributor Author

iccub commented Oct 10, 2019

I think I should add a preference to hide preview links, similar to what Safari does at the moment

@iccub iccub added the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Oct 10, 2019
@iccub iccub removed the blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue label Oct 11, 2019
@iccub
Copy link
Contributor Author

iccub commented Oct 11, 2019

Added content blocking to the link preview and an option to hide link previews

Zrzut ekranu 2019-10-11 o 14 53 41

Zrzut ekranu 2019-10-11 o 14 53 29

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Somehow after disabling link previews, I no longer got the enable link previews option… It seems like something is supposed to be there (you can see the menu has a final separator down there but no text/image.

IMG_C03DD67E96AA-1

@kylehickinson
Copy link
Collaborator

Ahhh, never mind, that list scrolls, and wasn't visible because of the super long URL… Not sure if we should truncate it?

@iccub
Copy link
Contributor Author

iccub commented Oct 11, 2019

Added a 100char truncate to it, Safari truncates it too btw

Zrzut ekranu 2019-10-11 o 15 59 53

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Works for me

@iccub iccub merged commit 1f6fe66 into development Oct 14, 2019
@iccub iccub deleted the bugfix/1499_link_preview branch October 14, 2019 12:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants