Add ability to share via links from the sidebar. #2215

Merged
merged 1 commit into from Jun 26, 2015

Projects

None yet

9 participants

@JakeHartnell
Contributor

Did a little bit more design work and thinking around sharing today, I'm submitting this PR as an alternative to #2012.

Here's what the new share dialog looks like:
Share dialog

The changes include:

  • Use the canonical url for generating via links.
  • Generate via uri in the share-dialog directive. (Thanks @csillag and @gergely-ujvari!)
  • Introduce toolbar share icon, the tooltip says "Share this page"
  • Update help/welcome page to cover sharing
  • Add simple sharing via Facebook, Twitter, Google plus, and email
  • Add social media icons to icomoon

Edit: We used have a share button on each annotation that links to the standalone annotation page. In the name of simplifying, I had removed it for the time being (thinking we should add it back when we can directly link to an annotation in context), but put it back after some outcry. Changes were made in a separate PR: #2231

@dwhly
Member
dwhly commented May 6, 2015

While I'm waiting for the dokku to build:

Does anyone violently disagree with that?

Yes, I do.

While I agree that direct linking is essential, and that the standalone page is probably what almost no one wants when wanting to share a link to the annotation, I'm quite passionately opposed to removing the share link to individual annotations/replies completely in the interim.

1- With this change there's zero way to link to specific annotations / replies, etc. at all. At least w/ the standalone page, one can point to a specific part of a conversation.
2- This is the canonical identifier for the annotation. In a sense, our whole narrative around how annotations are web resources is diminished by this change.
3- We probably always want some way to link to this, even when a direct linking approach is offered.
4- I talk to this share link all the time in demos, it's an important part of what we're trying to do.

Look forward to the rest of the PR, looks like some interesting thinking.

@tilgovi tilgovi commented on an outdated diff May 6, 2015
h/static/scripts/app-controller.coffee
@@ -46,6 +46,13 @@ module.exports = class AppController
oncancel = ->
$scope.dialog.visible = false
+ $scope.dialog = visible: false
+ $scope.viaLinkVisible = false
+
+ # Check to see if we are on the stream page.
@tilgovi
tilgovi May 6, 2015 Contributor

What's this change about?

@tilgovi tilgovi commented on an outdated diff May 6, 2015
h/static/scripts/config/via.coffee
@@ -0,0 +1,5 @@
+angular = require('angular')
+
+angular.module('h')
+.value('via', url: 'https://via.hypothes.is/')
+
@tilgovi
tilgovi May 6, 2015 Contributor

whitespace

@JakeHartnell
Contributor

Thanks for the nice response @dwhly, always good to get other perspectives. I think you've convinced me at least that it doesn't belong in this PR . Commit 72f83f5 re-adds the share button on annotations as things were before.

@tilgovi tilgovi commented on an outdated diff May 6, 2015
h/static/styles/simple-search.scss
@@ -3,6 +3,9 @@
.simple-search {
overflow: hidden;
+ position: absolute;
@tilgovi
tilgovi May 6, 2015 Contributor

Please try to find a way to do this layout without this. If it's possible to let the layout flow that's always better than absolute positioning in my mind.

@tilgovi tilgovi commented on an outdated diff May 6, 2015
h/templates/app.html
@@ -16,6 +16,7 @@
--><span class="provider" ng-show="auth.user">/{% raw %}{{ auth.user|persona:'provider' }}{% endraw %}</span><!--
--><i class="h-icon-arrow-drop-down"></i></span>
<ul class="dropdown-menu pull-right" role="menu">
+ <li ng-hide="isStream"><a href="" ng-click="viaLinkVisible = true">Share this page</a></li>
@tilgovi
tilgovi May 6, 2015 Contributor

If we have a share button in the search bar, let's maybe not have it in the dropdown.

@tilgovi tilgovi commented on an outdated diff May 6, 2015
h/templates/app.html
@@ -28,6 +29,15 @@
ng-click="login()"
ng-switch-when="null">Sign in</a>
+ <!-- Share this view -->
+ <span class="share-via-link">
+ <i class="h-icon-share"
+ title="Share this view"
+ ng-hide="isStream"
@tilgovi
tilgovi May 6, 2015 Contributor

Oh, I see. Fine for the moment.

@tilgovi
Contributor
tilgovi commented May 6, 2015

Generally, yes, I love the idea to share to these services directly from here.

I would reconsider copying my bad patterns, though ;).

I would ake the share button itself, the one in the top bar, part of the via link dialog directive and make that directive have a template that pops open and out of the top bar when it's open.

That saves a lot of nastiness:

  • The dialog is even closer to the button you click to open it
  • The markup can live in one place, rather than split over two (not counting the drop-down)
  • The app controller doesn't have to know about the state of the dialog

In short, the dialog can be self contained.

Try to get that nailed and I'll help you remove then need for isStream.

@jeremydean

is it relevant for me to ask here: why reddit and google plus?

does any really use the latter? and in terms of the former, does reddit really call for the kind of elegant share card i assume will populate on twitter? doesn't a naked link suffice there? note i'm not a reddit user.

also why not facebook?

@tilgovi
Contributor
tilgovi commented May 6, 2015

๐Ÿ‘ all of @jeremydean's questions.

@judell
Contributor
judell commented May 6, 2015

Late to this party, sorry. https://sharevia.dokku.hypothes.is/ down now?

On Tue, May 5, 2015 at 9:34 PM, Randall Leeds notifications@github.com
wrote:

[image: ๐Ÿ‘] all of @jeremydean https://github.com/jeremydean's
questions.

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@tilgovi
Contributor
tilgovi commented May 6, 2015

Probably. That's my fault. I ran some upgrades on dokku and ended up wiping
the docker partition when I ran into mega problems.
On May 6, 2015 12:36 PM, "judell" notifications@github.com wrote:

Late to this party, sorry. https://sharevia.dokku.hypothes.is/ down now?

On Tue, May 5, 2015 at 9:34 PM, Randall Leeds notifications@github.com
wrote:

[image: ๐Ÿ‘] all of @jeremydean https://github.com/jeremydean's
questions.

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@JakeHartnell
Contributor

Probably. That's my fault. I ran some upgrades on dokku and ended up wiping the docker partition when I ran into mega problems.

That's why I've had trouble pushing out a dokku for this... I keep getting:

To dokku@dokku.hypothes.is:sharevia
 ! [remote rejected] sharing-via-link-from-extension -> master (pre-receive hook declined)
error: failed to push some refs to 'dokku@dokku.hypothes.is:sharevia'
@tilgovi
Contributor
tilgovi commented May 6, 2015

Well if that's still the case after you recreate the app let me know.

The upgrades were partly because I was experiencing similar.
On May 6, 2015 12:43 PM, "Jake Hartnell" notifications@github.com wrote:

Probably. That's my fault. I ran some upgrades on dokku and ended up
wiping the docker partition when I ran into mega problems.

That's why I've had trouble pushing out a dokku for this... I keep getting:

To dokku@dokku.hypothes.is:sharevia
! [remote rejected] sharing-via-link-from-extension -> master (pre-receive hook declined)
error: failed to push some refs to 'dokku@dokku.hypothes.is:sharevia'

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@judell judell referenced this pull request May 6, 2015
Closed

[hack day] Search sharing (with VIA) #2193

2 of 7 tasks complete
@judell
Contributor
judell commented May 6, 2015

This got me pondering direct linking again. #2193 envisions a server-side API that persists annotations sets. Would the simplification proposed there -- just pass an annotation id straight through via into the app -- get us to MVP on this?

There's a world of difference between this:

http://jonudell.net/images/wordnik-balloon.jpg

and this:

https://hypothes.is/a/JdjfuZBCTJWXk5NYnhTJBg

Jake's instinct is correct, the latter is lame. Dan's right too, we currently rely on it.

There's already some momentum on #2193, it isn't in the current sprint whereas #2012 (and maybe #2215 now too), can/should we combine efforts?

@judell
Contributor
judell commented May 6, 2015

I will answer my own question: Not now. One step at a time.

@JakeHartnell
Contributor

@jeremydean, I didn't include Facebook (at first -- it is now included) because their documentation only described adding a share button to the page with JavaScript. I didn't want facebook Javascript to load on all the pages the extension does, but I eventually found what I needed: a link we can pass parameters into (like URL).

@csillag
Contributor
csillag commented May 7, 2015

There's already some momentum on #2193, it isn't in the current sprint whereas #2012 (and maybe #2215 now too), can/should we combine efforts?

Yes, those should definitely be combined.
But let's finish this first, and then build #2215 on top of this.

@csillag
Contributor
csillag commented May 7, 2015

-Introduce toolbar share icon as well as a username menu item (both are hidden on the steam)
toolbar icon

I think having the share icon next to the search bar is slightly confusing; some people might get the mistaken idea that the thing being shared has something to do with the search query or search results ... which will be supported with #2193 (hopefully), but not right now.

@judell
Contributor
judell commented May 7, 2015

Note for future enhancement: A user setting to choose which share buttons to display by default. LinkedIn will matter in some circles, Reddit in others, etc. No need to be constantly facing a button you'll never use.

@tilgovi
Contributor
tilgovi commented May 7, 2015

Still wish this had worked out: https://github.com/xauth/xauth

@JakeHartnell
Contributor

some people might get the mistaken idea that the thing being shared has something to do with the search query or search results ... which will be supported with #2193 (hopefully), but not right now.

That is exactly what I was going for. : ) When #2193 is done, we should change the text of the tool tip and dialog to "share this view" or something similar. I first had this idea as I was working on groups and also needed a quick to get the link to the group again, so putting the share button in the toolbar seemed like a natural way to grab a link to what every you are looking at: a page, a search query, or soon even a group. Still need to do user testing naturally.

@JakeHartnell
Contributor

Note: the screen shot is out of date, the tooltip when hovering over the share icon says: "Share this page." When #2193 is done we can change it.

@judell
Contributor
judell commented May 7, 2015

What's up with https://sharevia.dokku.hypothes.is/?

On Thu, May 7, 2015 at 11:16 AM, Jake Hartnell notifications@github.com
wrote:

Note: the screen shot is out of date, the tooltip when hovering over the
share icon says: "Share this page." When #2193
#2193 is done we can change it.

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@judell
Contributor
judell commented May 7, 2015

@tilgovi https://github.com/xauth/xauth Yeah, the field is littered with corpses. The IIW (Internet Identity Workshop) folks refer to what xAuth wanted to solve as the NASCAR problem

@tilgovi
Contributor
tilgovi commented May 14, 2015

If this is nearing ready it needs a rebase.

@JakeHartnell
Contributor

Rebased. This just needs review, in which a dokku would be very helpful! (Still having problems there.)

I've made some last minute improvements after doing a bit of usability testing, mostly adding readonly to the input with the via link so that people don't accidentally change or delete the link they are trying to copy. : )

Cleaned up the CSS a bit too.

Lastly, I played around with the copy and order of the dialog, it now looks/reads like this:
Share dialog

@tilgovi
Contributor
tilgovi commented May 14, 2015

Looking good. Copy could still be simpler, to me.

"Share the link below to show anyone these annotations and invite them to
contribute their own."

I don't think you need more than that. The bit about loading Hypothesis is,
IMO, too much technical detail. Words like "loaded" totally unnecessary.
Words like 'click' are mouse-centric.

Lastly, a question about the links. Is "post" preferred to "share" for
Facebook but not G+ for some reason?

Maybe a verb on the email (I know, email can be a verb) but maybe "Send via
email" to balance out the line lengths a bit.

Also, it may still be the case that more people share articles by email
than any other way. Do a quick search, this is a not infrequently reported
on thing. The answer is out there. And my bad if you already did this. But
if true, maybe email should be at the top.
On May 14, 2015 3:07 PM, "Jake Hartnell" notifications@github.com wrote:

Rebased. This just needs review, in which a dokku would be very helpful!
(Still having problems there.)

I've made some last minute improvements after doing a bit of usability
testing, mostly adding readonly to the input with the via link so that
people don't accidentally change or delete the link they are trying to
copy. : )

Cleaned up the CSS a bit too.

Lastly, I played around with the copy and order of the dialog, it now
looks/reads like this:
[image: Share dialog]
https://cloud.githubusercontent.com/assets/521978/7643027/af517e5a-fa4a-11e4-9cec-5d6662a321ce.png

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@tilgovi
Contributor
tilgovi commented May 14, 2015

I'm skeptical of the style changes but I can check it out if you like. What's the max-width for? Does the link need to float left? Why?

Should we really be changing all the dialog stuff like this, or should that be better encapsulated in the directive.

I'd like to get the dialog template not to have the top level wrapper that it has. That makes it too aware of its placement and as such less relocatable as a component. It also introduces noise into the diff, including the changes to distinguish it from the other dialog that should also not be bleeding all over the AppController.

I know a lot of this is that I did things poorly with the account controller and now you're copying me, so no worries, but let's always be improving.

If you need help making these changes, just ask, or pair with someone on it.

@judell
Contributor
judell commented May 15, 2015

A quick survey:

image

At this point the icons are, well, iconic. Text is minimized to the point that some of these sites don't even bother with tooltips.

Available choices, and the ordering of them, varies according to audience. Since we appeal to no specific audience I don't think there's a right answer, and would eventually want our users to be able to configure both choices and order.

@tilgovi
Contributor
tilgovi commented May 15, 2015

Ah, this is what I was looking for: http://www.sharethis.com/blog/2015/01/21/q4-2014-consumer-sharing-trends-report/#sthash.P2hBdzEp.dpbs

It seems that things have changed in the last few years and email has lost ground. No surprises.

Anyway, I'm also with @judell about possibly not needing any text for these.

Let's fiddle with the design and see how clean this can be.

@JakeHartnell
Contributor

Ok, dokku is finally up here: https://sharevia.dokku.hypothes.is

Thanks @tilgovi and @judell for the many suggestions, sorry I've been a bit slow to respond. I've made another iteration on the design based on your wonderful thoughts, which now looks like this:
share via link

Thoughts?

@judell
Contributor
judell commented May 18, 2015

+1.

@judell judell added this to the 05-19-2015 milestone May 18, 2015
@tilgovi
Contributor
tilgovi commented May 18, 2015

So nice.

@JakeHartnell
Contributor

Rebased.

@JakeHartnell
Contributor

I'd like to get the dialog template not to have the top level wrapper that it has. That makes it too aware of its placement and as such less relocatable as a component. It also introduces noise into the diff, including the changes to distinguish it from the other dialog that should also not be bleeding all over the AppController.

Feels like a different issue I think (since it touches on both the account, sharing, and future dialogs), but one we should certainly work on. I do agree that we need a better way to do dialogs, and I'm happy to work on that later.

Anything else holding things up?

@tilgovi
Contributor
tilgovi commented May 19, 2015

Let me take a quick look at that later today.
On May 19, 2015 2:22 PM, "Jake Hartnell" notifications@github.com wrote:

I'd like to get the dialog template not to have the top level wrapper that
it has. That makes it too aware of its placement and as such less
relocatable as a component. It also introduces noise into the diff,
including the changes to distinguish it from the other dialog that should
also not be bleeding all over the AppController.

Feels like a different issue I think (since it touches on both the
account, sharing, and future dialogs), but one we should certainly work on.
I do agree that we need a better way to do dialogs, and I'm happy to work
on that later.

Anything else holding things up?

โ€”
Reply to this email directly or view it on GitHub
#2215 (comment).

@BigBlueHat
Contributor

@RawKStar77 couple things. I agree with @csillag's point about the odd placement of the share button. There's nothing else "click-able" in that region except the close sidebar button--which is a very different intent.

Given your thoughts on "eventually this will share the current view" what do you think about moving the share icon down into the gray space floated right of the "Sorted on location" drop down.

Like this (or similar):
2015-05-23 11_41_31-hypothesis
(obviously with some tweaking ๐Ÿ˜‰)

That would move it in the same conceptual space as the filter, and the same click-focused region as the user drop down list and other "action" click-ables.

Also, it'd be best (for similar reasons) to right align the share buttons (so they match all the other buttons) and also make them match the same color/hover experience as the other buttons.

Let me know if you want to chat through these or anything. Cheers!

@dwhly
Member
dwhly commented May 23, 2015

The "social" view that will be the primary context for sharing (when groups is implemented) is up in the toolbar above tho.

@dwhly
Member
dwhly commented May 23, 2015

I suppose it could move down as well, but they should probably stay close together.

@BigBlueHat
Contributor

That one makes sense where it is:
2015-05-23 12_03_00-hypothesis

Putting the group name (and I assume eventually a drop-down to change it?) has similar meaning to the username: "I am taking action as / for this Entity."

The share button (even in the groups demo), still feels off all by itself in an area one isn't looking to click--except to type in a search.

@dwhly
Member
dwhly commented May 23, 2015

The share button (even in the groups demo), still feels off all by itself in an area one isn't looking to click--except to type in a search.

Yeah, I think the reasoning is that the share button should be next to filters from which it will inherit context. Search being one of those. How best to do that is the question.

@dwhly
Member
dwhly commented May 23, 2015

Future thought: it might be nice if the Share dialog could explicitly list which filter values it was capturing (at the point that these are implemented).

@BigBlueHat BigBlueHat referenced this pull request May 23, 2015
Closed

Add feed icon to sidebar #2252

0 of 4 tasks complete
@tilgovi
Contributor
tilgovi commented May 26, 2015

So, what's actually being shared? Here, it's the page.

Have we considered share as a toolbar icon, of the variety that floats inside the page even when the sidebar is closed?

@JakeHartnell
Contributor

Have we considered share as a toolbar icon, of the variety that floats inside the page even when the sidebar is closed?

Yes, that was the first thing I considered, and works well for sharing just a page, but I chose the current design over that option because I had a thought towards the future when we could share not only just a page, but the current view: filters, search queries, groups, etc. I also think by having the share icon in the sidebar makes it ultra clear that we are sharing this layer over the page and not the page itself.

@tilgovi
Contributor
tilgovi commented May 26, 2015

I wonder if that actually just muddles things. Would we necessarily not want two entry points to sharing, in different places and thus implying different context, if we introduce other features? For the purposes of this one, though, does the toolbar really not make sense?

If it isn't sharing the page with annotations, it wouldn't be there at all. Users can copy/paste the location bar or use whatever in-browser sharing feature they usually would.

Even if it were to surprise them that it shares the page with annotations, aren't we aiming to be so unobtrusive as to have that not be an unpleasant surprise?

Is there a scenario where having it in the toolbar would rightly anger a user?

@tilgovi
Contributor
tilgovi commented May 26, 2015

In any case, if we think there's potential confusion should we consider instead to make the whole thing more explicit and have language for it somewhere?

I'm not trying to hard sell any one design, but I'm asking questions. I don't see enough of the thinking that has happened yet to see anything that really justifies the search bar as the best place for this to me.

@judell
Contributor
judell commented May 27, 2015

"Is there a scenario where having it in the toolbar would rightly anger a user?"

Now that it's possible to leave the (test) extension active I have been doing so. As a result, I notice when it obscures things and am annoyed by that. Growing the toolbar will obscure more things and annoy me more.

I am concerned about clutter, and while #2252 puts even more pressure on scarce space, I think it's necessary and appropriate because it's really about notification, not just RSS.

Here's a legend to help think about these things.

[<] = share control (our share icon)

[R] = notify control (R denotes RSS icon)

[O] = view control (O denotes the globe icon for All or Public)

[A] = app control (the app menu)

Each is clickable, each expands to offer affordances:

[<] = open a panel, offer a page-level share link (possibly scoped by [O])

[O] = open a dropdown list, offer to select a page-level scope

[R] = open a panel, offer page-level notification (by RSS/Atom, email, possibly other)

[A] = open a menu, offer app-level actions

The odd man out here is actually search. On a call just now we talked about suppressing the search box until the search icon is clicked. Straw man proposal: do that but then open a panel for search, within which we can also offer the currently-undocumented search affordances (user and tag facets).

Let's call that one [S]. So now we have:

[S] = search control

[R] = notify control (R denotes RSS icon)

[<] = share control (our share icon)

[O] = view control (O denotes the globe icon for All or Public)

[A] = app control (the app menu)

We can debate the order of these things but what we have here is a top-level app menu where everything item is a subsidiary interaction.

(I recognize that [R] is out of scope for this PR but a notification control is coming so let's factor that into the thinking here.)

@JakeHartnell
Contributor

Sharing is important, it deserves to have visual prominence in our app, and I think the best place for that is in the top bar. Jon hit on a key point, which is that we need to be forward thinking about what other UI elements we are going to be adding to the app soon (such as notifications and needing to grab a link with filters, queries, or groups applied).

I do not think we should put the share dialog toggle in the toolbar as @tilgovi has suggested, even though I confess this was also one of my first thoughts. However, I eventually decided against the toolbar as I don't want to be continually adding things to it. I agree with @judell:

As a result, I notice when it obscures things and am annoyed by that. Growing the toolbar will obscure more things and annoy me more.

Are we going to add notifications to the tool bar? What else? I'm not a big fan of adding things to the toolbar unless we absolutely have to.

I think putting things in the topbar is a better alternative, as there is more space, and again with an eye towards the future where we will be able to share search queries, groups, or filters. Here's an example from the groups prototype:
Groups prototype sharing

I can imagine something similar with search parameters, and I think having the share dialog toogle in the topbar (as proposed by this PR) will make it clearer what is happening when we have this functionality... but time and users will give us much more information.

Additionally, IMO having the share icon in the sidebar makes it ultra clear that we are sharing this layer over the page and not the page itself. For what it's worth, in informal user tests with a few of my friends no one has failed to share the page yet.

@BigBlueHat's proposal was quite tempting, but I think the top bar is a better place for the ultra important activities of sharing and notifications. It's important that it's fixed too, since it is still selectable when scrolling through annotations:
Example

As far as space in the top bar goes, I'm not to worried. I think eventually we should pick up on @aron's idea to implement a new filter bar design: #1562 as it will address the problem of not having filters accessible when scrolling through annotations an provide us with a bit more space.

As for @tilgovi's point of making the whole thing more explicit and having language for it 'somewhere.' I've added a new section to the help page. The icon is also fairly recognizable, and it has a tooltip... no different than it would be if it were in the toolbar. Originally, I also had a separate menu item in the dropdown user menu that read "Share this page," though I removed it after someone questioned the need for the repetition. I admit I was on the fence about this as well.

I shall also make the point that this is by no means the end-all 'final' design of how sharing (or even the topbar) will look in our application. We are going to add sharing back on the cards when we have direct linking, and we should be exploring other options too (such as what sharing might look like on the stream?).

I feel like we can talk about the placement of this icon to death, when in reality is very easy to move and rather than wasting too much moving it to various places I would prefer continuing with the current design, get it in front of users, and iterate. Does anyone strongly object?

@judell
Contributor
judell commented May 27, 2015

"get it in front of users, and iterate"

+1

@nickstenning
Member

This has been open forever, and it's now looking pretty good. I'd love to merge it, but it doesn't seem to have any tests. Could you add some tests for the share-dialog directive?

@JakeHartnell
Contributor

@nickstenning, I added a simple test to App-Controller-Test.coffee. There's probably more that can be added with regard to to this part of the share-dialog directive:

        scope.$watchCollection (-> crossframe.providers), ->
            if crossframe.providers?.length
                # XXX: Consider multiple providers in the future
                p = crossframe.providers[0]
                if p.entities?.length
                    e = p.entities[0]
                    scope.viaPageLink = 'https://via.hypothes.is/' + e

But I'm not quite sure how to test that. Any suggestions? @seanh, I know you've been working a lot with tests of late.

Should I make a fake crossframe.providers collection and then add something to it and see if viaPageLink is set?

@nickstenning
Member

Ok @RawKStar77, yes, you need to add some tests for the directive. So create a h/static/scripts/directive/test/share-dialog-test.coffee file -- status-button-test.coffee looks like a good example to copy.

You can fake out the crossframe service. There's an example of this in h/static/scripts/test/widget-controller-test.coffee (search for fakeCrossFrame in that file) and copy the pattern in your tests.

Then you want some tests which set crossframe.providers to some values you might expect, and ensure that $scope.viaPageLink is updated appropriately. You'll probably have to manually call $scope.digest() each time you change crossframe.providers.

Feel free to grab me on IRC if you have any problems.

@JakeHartnell
Contributor

Ok, after some help from @nickstenning in the test writing department, this is ready. Someone want to push the button?

@nickstenning nickstenning and 1 other commented on an outdated diff Jun 16, 2015
h/static/scripts/directive/share-dialog.coffee
@@ -0,0 +1,28 @@
+###*
+# @ngdoc directive
+# @name Share Dialog
+# @restrict A
@nickstenning
nickstenning Jun 16, 2015 Member

This line is out of date.

@JakeHartnell
JakeHartnell Jun 16, 2015 Contributor

Good catch, fixed.

@tilgovi tilgovi referenced this pull request Jun 17, 2015
Closed

"Via" bookmarklet #2325

@JakeHartnell
Contributor

Bump. @nickstenning, @tilgovi, or @seanh... is there anything else I need to do or can we hit the button?

@tilgovi tilgovi commented on the diff Jun 19, 2015
h/static/scripts/directive/share-dialog.coffee
@@ -0,0 +1,29 @@
+###*
@tilgovi
tilgovi Jun 19, 2015 Contributor

This file is inconsistent with the rest of our CoffeeScript. It has four space indents instead of two.

@JakeHartnell
JakeHartnell Jun 19, 2015 Contributor

Ah yes, sorry about that. Fixed.

@tilgovi tilgovi commented on an outdated diff Jun 19, 2015
h/static/scripts/test/app-controller-test.coffee
@@ -117,7 +117,11 @@ describe 'AppController', ->
it 'does not show login form for logged in users', ->
createController()
- assert.isFalse($scope.dialog.visible)
+ assert.isFalse($scope.accountDialog.visible)
+
+ it 'does not show shareDialog by default', ->
@tilgovi
tilgovi Jun 19, 2015 Contributor

Try to write these in a natural language descriptive style, rather than referencing the implementation. 'does not show the share dialog at start' or something. I'm totally nitpicking, but shareDialog is an object, it can't be shown. The "share dialog" is a thing that can be shown.

@tilgovi tilgovi commented on the diff Jun 19, 2015
h/static/styles/simple-search.scss
@@ -9,7 +9,7 @@
@include clearfix;
position: relative;
padding: 0 1.5385em;
- color: $gray;
+ color: $gray-darker;
@tilgovi
tilgovi Jun 19, 2015 Contributor

The changes to the colors in this file could be a separate PR, maybe.

@JakeHartnell
JakeHartnell Jun 19, 2015 Contributor

It's to make them consistent with the new share icon, so I think it's related.

@tilgovi tilgovi commented on an outdated diff Jun 19, 2015
h/static/styles/simple-search.scss
}
input.simple-search-input {
float: left;
outline: none;
color: $text-color;
- width: 100%;
@tilgovi
tilgovi Jun 19, 2015 Contributor

Are we sure this causes no problems? I thought it was necessary or we'd end up with a short field.

@tilgovi tilgovi and 1 other commented on an outdated diff Jun 19, 2015
h/static/styles/topbar.scss
@@ -41,7 +41,7 @@
& > *:first-child, & > *:last-child {
line-height: 28px;
- margin: 0 .72em;
+ margin-right: .72em;
@tilgovi
tilgovi Jun 19, 2015 Contributor

This doesn't seem right. It feels like, if we want to have no margin on the share button, that this should be an override done by the share button. The default for the topbar (on the stream and other pages) is definitely to have symmetric margins.

@JakeHartnell
JakeHartnell Jun 19, 2015 Contributor

Yeah, but the reason I changed this is because it makes the search icon look off on the stream. You can see this live on hypothes.is/stream:
selection_089

See how the search icon doesn't line up with Sorted.

@tilgovi
tilgovi Jun 19, 2015 Contributor

Then you should adjust the CSS around media queries and the topbar to make this a symmetric change.

@tilgovi
tilgovi Jun 19, 2015 Contributor

What I mean is that if you resize the view to respond to a smaller screen you'll see that it does line up. Make that always the case.

@tilgovi tilgovi commented on an outdated diff Jun 19, 2015
h/templates/app.html
@@ -28,6 +28,14 @@
ng-click="login()"
ng-switch-when="null">Sign in</a>
+ <!-- Share this page -->
+ <span class="share-dialog-toggle" ng-hide="isStream">
@tilgovi
tilgovi Jun 19, 2015 Contributor

I'd give this its own variable. isStream is handled by the route controllers. It's a bare property on the scope, rather than a nested property of a scope object, and so even though it's not modified it still goes against the best practice of not shadowing scope variables.

I'd just be explicit about this. showShareButton or something.

In the future, we will eventually follow through on separating the stream and viewer widget templates, and maybe the share dialog will move into the latter. But for now, let's at least keep our language clear and not conflate things.

@tilgovi tilgovi commented on the diff Jun 19, 2015
h/templates/client/share_dialog.html
@@ -0,0 +1,29 @@
+<div id="dialog" class="sheet content">
@tilgovi
tilgovi Jun 19, 2015 Contributor

Indentation in this template is inconsistent with the rest of the project.

@tilgovi tilgovi commented on an outdated diff Jun 19, 2015
h/templates/help.html
@@ -84,8 +84,8 @@ <h3 class="feature-heading"><i class="feature-icon h-icon-reply"></i> Replies</h
<h2 id="key-features" class="help-page-heading">Privacy</h2>
<p class="help-page-lede">Annotations are either public and viewable by everyone
or private and visible only to you.</p>
- <div class="grid feature-content">
- <section class="feature column-desktop-1-2">
@tilgovi
tilgovi Jun 19, 2015 Contributor

Is this change intentional?

@tilgovi
Contributor
tilgovi commented Jun 19, 2015

@JakeHartnell gave some detailed feedback on some things. Most should be easy to change. I think the CSS for the share button smells a bit, but if you don't see an easy way to improve it I'm happy to take a look.

@JakeHartnell
Contributor

@tilgovi I made the changes you requested.

I think the CSS for the share button smells a bit, but if you don't see an easy way to improve it I'm happy to take a look.

The CSS for the whole topbar smells a bit IMO. As we are adding more things to the top bar, we're going to have to work on the CSS for it at some point, because with all the different floats going on, things are getting crazy. I think a rewrite is out of scope for this PR though, and I want to get this in front of users for feedback sooner rather than later. : )

@tilgovi
Contributor
tilgovi commented Jun 19, 2015

I didn't say we needed to write the whole top bar. It may be that we have to. If you don't know how to improve it without a rewrite, that's fine. That's why I offered to take a look, which I'll now do.

@JakeHartnell JakeHartnell Add ability to generate via links from the sidebar.
- Use document plugin to get canonical url for sharing.
- Introduce Topbar share icon
- Add social media icons to icomoon
- Generate via uri in the share-dialog directive
- Add sharing via Facebook, Twitter, Google plus, and email
- Make account dialog and share dialog consistent
- Add information about sharing to the help page.
- Add tests (Thanks @nickstenning!)
d3a03e8
@JakeHartnell
Contributor

Thanks @tilgovi.

@tilgovi tilgovi self-assigned this Jun 22, 2015
@tilgovi tilgovi added 2 - In Progress and removed 3 - Done labels Jun 22, 2015
@JakeHartnell
Contributor

Bump. Can I get either explicit feedback about what I need to do to get someone to push the merge button, or can we merge and then re-factor later (which we will be doing anyway soon, there is much more work to do with regards to sharing)?

@tilgovi
Contributor
tilgovi commented Jun 26, 2015

Sure.

@tilgovi tilgovi merged commit f84dd07 into master Jun 26, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tilgovi tilgovi deleted the sharing-via-link-from-extension branch Jun 26, 2015
@JakeHartnell
Contributor

Thanks @tilgovi!

@robertknight
Contributor

I couldn't find the share facility anywhere until I read this bug report just now. I completely missed the positioning next to the search bar.

I was looking for it alongside the the rest of the actions relating to a specific annotation, I think for a couple of reasons:

  • They are red so they function as a visible call-to-action
  • I wanted to share not just the annotations on the whole page but a link that would direct someone to a specific annotation within the page.
@dwhly
Member
dwhly commented Aug 12, 2015

I was looking for it alongside the the rest of the actions relating to a specific annotation, I think for a couple of reasons.

We actually had a share link on the annotation card for a while, in the place of the link icon now-- which simply exposes a link to the standalone page for the annotation.

We plan to reintroduce it when direct linking (hypothesis/vision#87) is finished. (It's near the top of the backlog now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment