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

Jump to bottom

Use <web-share-wrapper> for article sharing #1524

Merged
merged 9 commits into from Feb 11, 2019
Merged

Use <web-share-wrapper> for article sharing #1524

merged 9 commits into from Feb 11, 2019

Conversation

philnash
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

This is an initial attempt at replacing share links with the web share API on supported platforms. It uses <web-share-wrapper>, a custom element that only takes over if the web share API is available (currently Chrome on Android, but it's in preview in Safari too).

One question: I'm not sure this is the best way to include the module. Ideally it would just sit in the vendor bundle, but I couldn't work out how to get it in there. Any help on that would be appreciated.

Related Tickets & Documents

#399

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

screenshot_20190112-143333
screenshot_20190112-143505

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 12, 2019
@Link2Twenty
Copy link
Contributor

How does it look on a smaller device, phone sized?

@philnash
Copy link
Contributor Author

How does it look on a smaller device, phone sized?

Much the same, I just don't have a mobile Android device to hand to screenshot right now.

@bennypowers
Copy link
Contributor

Please see #1579 which adds the webcomponents polyfill. Perhaps that one should be added in a more central place?

@bennypowers
Copy link
Contributor

@Link2Twenty It's not a web-based modal dialog, it actually activates the device's native share pane:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/share

So how does it look? the same way shares look from whatsapp, telegram, or mastodon - the way the OS UI designers intended.

bennypowers
bennypowers reviewed Jan 17, 2019
View reviewed changes
Copy link
Contributor

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

Looks awesome. nice work.

app/javascript/packs/webShare.js Show resolved Hide resolved
app/views/layouts/application.html.erb Outdated Show resolved Hide resolved
@Link2Twenty Link2Twenty mentioned this pull request Jan 26, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Feb 4, 2019
Closed
@philnash
Copy link
Contributor Author

philnash commented Feb 5, 2019

Can I get a team member to review this or let me know what they might be thinking about it? I don't seem to be able to request reviews.

Zhao-Andy
Zhao-Andy approved these changes Feb 6, 2019
View reviewed changes
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Looks great and works great!

Screenshot on my smaller Android device:
screenshot

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 6, 2019
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 8, 2019
maestromac
maestromac requested changes Feb 8, 2019
View reviewed changes
Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

Thank you for this pr @philnash ! I've tested it on my android and it works really well.

app/views/articles/_actions.html.erb Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 8, 2019
@weekly-digest weekly-digest bot mentioned this pull request Feb 11, 2019
Closed
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Feb 11, 2019
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 11, 2019
@maestromac maestromac merged commit d8de2b0 into forem:master Feb 11, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 11, 2019
@maestromac maestromac mentioned this pull request Feb 11, 2019
Closed
@maestromac
Copy link
Member

Hey @philnash would you like to write up a changelog on dev.to?

@weekly-digest weekly-digest bot mentioned this pull request Feb 18, 2019
Closed
@philnash
Copy link
Contributor Author

Glad this got merged, thanks everyone!

Sorry I didn't get back to you @maestromac, was on holiday and avoiding computers/emails/etc. Will likely write up a post about this soon though.

@maestromac maestromac mentioned this pull request Apr 22, 2019
Closed
This was referenced Aug 23, 2020
Closed
Merged
@philnash philnash mentioned this pull request Oct 7, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennypowers bennypowers bennypowers left review comments

@maestromac maestromac maestromac approved these changes

@Zhao-Andy Zhao-Andy Zhao-Andy approved these changes

Assignees
No one assigned
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@philnash @Link2Twenty @bennypowers @maestromac @Zhao-Andy @benhalpern