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

x/pkgsite: provide multiple "copy to clipboard" options #38514

Open
quentinmit opened this issue Apr 18, 2020 · 18 comments
Open

x/pkgsite: provide multiple "copy to clipboard" options #38514

quentinmit opened this issue Apr 18, 2020 · 18 comments

Comments

@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Apr 18, 2020

What is the URL of the page with the issue?

Any package page (e.g. https://pkg.go.dev/github.com/sirupsen/logrus?tab=doc)

What is your user agent?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.92 Safari/537.36

Screenshot

Screen Shot 2020-04-18 at 3 24 01 AM

What did you do?

Click the "Copy path to clipboard" button

What did you expect to see?

github.com/sirupsen/logrus on my clipboard

What did you see instead?

import "github.com/sirupsen/logrus" on my clipboard

I almost never want an import statement; I want something I can copy into a go get command in my terminal. Plus all the UI indicates it's a path, not an import statement.

@gopherbot gopherbot added this to the Unreleased milestone Apr 18, 2020
@gopherbot gopherbot added the pkgsite label Apr 18, 2020
@ALTree
Copy link
Member

@ALTree ALTree commented Apr 18, 2020

The import prefix was added to fix #36809 after @myitcv requested it.

@obeyda
Copy link
Contributor

@obeyda obeyda commented Apr 18, 2020

Isn't it possible to have both?
maybe two, or even three views,

  • one for the package only github.com/sirupsen/logrus
  • one for the import path import "github.com/sirupsen/logrus"
  • and maybe one for the get statement go get github.com/sirupsen/logrus
@ALTree
Copy link
Member

@ALTree ALTree commented Apr 18, 2020

@obeyda Three "copy to clipboard" buttons are about two too many.

@obeyda
Copy link
Contributor

@obeyda obeyda commented Apr 18, 2020

something like this maybe
image
we can have small buttons to switch between views,

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Apr 18, 2020

Sharing some additional relevant information, in case it's helpful.

Gerrit has a "Download" button. Pressing it opens a dialog where multiple text boxes appear, each with their own copy-to-clipboard button:

image

I'm not sure that more than 1 way of copying the import path is really needed, but if so, I've found the above UI to work well to solve that problem.

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 18, 2020

@obeyda the use case and rationale behind the decision in #36809 was that most of the time people will be copying an import path to then paste directly into code.

Please can you provide some details on your use case here?

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 19, 2020

I can tell you the current behavior bugs me greatly, as either:

  • I really want to insert the import into an existing import block, not add a new one. Even though gopls will end up "cleaning up" the problem via some quickfix, it's strange to do something so differently than I would normally do when adding a new import.
  • I want to go get the package at my terminal, so do not want the import keyword or quotes. Right now either I can select the package name from the header, or attempt to extract the package name from the URL bar (which is much more difficult on pkg.go.dev as it forces query params for the tabs). go get -d -ing a package is the main way I get a dependency I've never used before into my cache and easily available as a suggestion in gopls.
  • I want to go to the repo for the package faster than trying to figure out how to get to the link on pkg.go.dev, which I can never remember is actually on the "module" page, which is not a tab, but a link at the top.

Having a clipboard button next to a package name, then clicking it and the clipboard containing something entirely different than what I thought I was copying is not my preferred UI. If a "copy to clipboard button" is next to a piece of text, I expect to copy that text. I have to remember that it's going to do this, and select the name I want somewhere else, eliminating the button from my workflow entirely.

godoc.org doesn't have this problem. It has the import statement at the top, where someone could select just the name or the import as desired, or use the URL bar (which contains no query params).

I just want the button to do "what it says on the tin".

@quentinmit
Copy link
Contributor Author

@quentinmit quentinmit commented Apr 19, 2020

I thought I already made my usecase clear, the only time I ever copy the package name is to paste it into my terminal after "go get". goimports means I ~never need to write an import statement by hand.

@obeyda
Copy link
Contributor

@obeyda obeyda commented Apr 19, 2020

@myitcv, what i suggested was something to make the UX a user friendly.
from the comments above and the other issue (#36809) we can see that different users have different use cases.

@tooolbox
Copy link

@tooolbox tooolbox commented Apr 20, 2020

Fully agreed with @zikaeroh

Key points:

  • The behavior is surprising.
  • Copying the path could be followed by go get or pasting it into a go.mod or chatting it to your buddy or anything else; assuming an import alienates other use cases.

Please consider reverting.

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 20, 2020

@quentinmit

I thought I already made my usecase clear

Apologies, I totally missed this in the description.

That makes total sense as a use case. As does, I would suggest, wanting to paste directly into code. i.e. we have multiple use cases.

So as I understand it we have a number of options (and at this point I defer to @julieqiu and team) not limited to:

  • if we can only have one copy option, go with the use case that is most useful to the majority
  • if we can have a UI/UX that allows multiple options (per @obeyda and @dmitshur) then provide multiple options

Please consider reverting.

I don't think this is a necessary step, per my points above. There are an unknown number of people who are happy with the current value (modulo the point below about the poor UX of the copied value being different from what is shown) who are not providing feedback here.

@zikaeroh

I really want to insert the import into an existing import block

The motivation behind inserting the import keyword is that (in a world with a single copy-paste option) it is generally useful in all cases where you are pasting into code. Not having the import keyword is less useful in the case where I don't have any existing imports. As you say, tools like goimports (via gopls) do this tidying for us.

I want to go get the package at my terminal

I understand and empathise with the use case. Per my comments above, assuming we can and want to support this in the UI/UX, this doesn't need to be an exclusive choice between use cases.

I want to go to the repo for the package faster than trying to figure out how to get to the link on pkg.go.dev, which I can never remember is actually on the "module" page, which is not a tab, but a link at the top.

I would suggest this is worthy of feedback in a separate issue; however, if you want to use the copy-pasted value from an unadorned import path for other reasons, you'll continue to be able to in a world with multiple options via UI/UX wizardry.

Having a clipboard button next to a package name, then clicking it and the clipboard containing something entirely different than what I thought I was copying is not my preferred UI.

This is a good point and one I 100% agree with. It should be addressed. FWIW the motivation behind wanting a different copy-paste value came from godoc.org, where the full import directive is shown:

Screen Shot 2020-04-20 at 14 53 02

@tooolbox
Copy link

@tooolbox tooolbox commented Apr 20, 2020

I don't think this is a necessary step, per my points above. There are an unknown number of people who are happy with the current value (modulo the point below about the poor UX of the copied value being different from what is shown) who are not providing feedback here.

I get your point in that there has not been a broad survey of this, but in our sample size of the 8 people who've shown up on the issue tracker to talk about this, only 1 seems to be in favor of import X as the default copy value.

I agree wholeheartedly that it can be a mistake to please the vocal minority, but one wonders in this case which side that applies to.

The motivation behind inserting the import keyword is that (in a world with a single copy-paste option) it is generally useful in all cases where you are pasting into code. Not having the import keyword is less useful in the case where I don't have any existing imports. As you say, tools like goimports (via gopls) do this tidying for us.

Based on the various Go packages I have seen over the years, I'm willing to bet that if you took a survey of all Go code, you would find that the vast majority have multi-line imports statements with parens. Therefore it seems that in the majority of cases we are getting an extraneous import in our pasted value, no?

To me, it's very easy to:

  1. Put my cursor in the middle of a multi-line import statement.
  2. Type " which my editor converts to "|" (pipe is cursor)
  3. Paste the import path that I copied from the docs.

It's also very easy to type import " and then paste. It's easier to do that than do a paste, delete the quotes, and replace import with go get or what have you.

This is a good point and one I 100% agree with. It should be addressed. FWIW the motivation behind wanting a different copy-paste value came from godoc.org, where the full import directive is shown:

I think it was a good spot that this was dropped from pkg.go.dev and I do like having the import statement there in the Godoc version, but to me the value is communicating to the reader how the package is used. "Oh, so to use this I say import blah, okay I get it." I think the problem with #36809 was mixing "restore the import statement to the header" with the behavior of the new and beneficial copy-path feature.

One thought on this: an even more valuable display of import directive would be in cases where the package name doesn't match the path.

Let's take this example: https://github.com/graarh/golang-socketio which godoc shows the import for as import "github.com/graarh/golang-socketio" and yet the actual package name is gosocketio. It seems like you'd want to display (and copy) the import as:

import gosocketio "github.com/graarh/golang-socketio"

I can make this the subject of a separate ticket but I think it's worth discussing in the context of what's valuable about having the import path in the docs.

I understand and empathise with the use case. Per my comments above, assuming we can and want to support this in the UI/UX, this doesn't need to be an exclusive choice between use cases.

Yes, agreed, multiple use cases can be good.

So as I understand it we have a number of options (and at this point I defer to @julieqiu and team) not limited to:

  • if we can only have one copy option, go with the use case that is most useful to the majority
  • if we can have a UI/UX that allows multiple options (per @obeyda and @dmitshur) then provide multiple options

I think this is where the philosophies diverge. If we can only have one copy option, I believe we should go with the least common denominator (just the path) and users can type import or go get or whatever surrounding context they want.

There can be further debate, but I think the best solution here is to devise a header that enables:

  • Viewing the package path
  • Copying the package path to clipboard as the default or more prominent option
  • Viewing the import statement (possibly with the actual package name as I mentioned)
  • Copying the import statement (ideally this can be done without a bunch of additional clicks or modal popups)

(As a note, Godoc displays both the package path and the import statement, as you can see from that screenshot snippet.)

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 20, 2020

I agree wholeheartedly that it can be a mistake to please the vocal minority, but one wonders in this case which side that applies to.

I think you misunderstood my point. People are coming here because they also see a problem with the current behaviour. People who do not have a problem with the current behaviour are not coming here. So there is a selection bias. That there is a selection bias is not a problem, indeed it's totally to be expected. My point is simply that we need to be aware of that. Incidentally, I'm only commenting to give some context/rationale behind my previous issue on this.

But in any case, it's not worth dwelling on this point. We should instead focus on what we are solving for here.

Let me also be clear: my goal is not to defend the position I laid out in #36809. What I was and am trying to solve for is to help come up with the best possible solution, given various constraints. I'm absolutely not an expert in UI/UX design, indeed I'd prefer to defer to @julieqiu and team to help steer the process.

I think this is where the philosophies diverge. If we can only have one copy option, I believe we should go with the least common denominator (just the path) and users can type import or go get or whatever surrounding context they want.

I wouldn't describe myself as having a philosophy on this 😄 My list was absolutely not exhaustive (indeed I specifically made that point), rather it was intended to motivate the sort of discussion we're having now.

I now agree with your point that the least common denominator of a bare import path is almost certainly the "best" option if we only have a single copy UI/button/whatever, because it achieves exactly the goal of satisfying the greatest number of people with minimal impact on their workflow.

That said, I also agree with this as a proposed way forward:

I think the best solution here is to devise a header that enables:

  • Viewing the package path
  • Copying the package path to clipboard as the default or more prominent option
  • Viewing the import statement (possibly with the actual package name as I mentioned)
  • Copying the import statement (ideally this can be done without a bunch of additional clicks or modal popups)
@tooolbox
Copy link

@tooolbox tooolbox commented Apr 20, 2020

I think you misunderstood my point. People are coming here because they also see a problem with the current behaviour. People who do not have a problem with the current behaviour are not coming here. So there is a selection bias. That there is a selection bias is not a problem, indeed it's totally to be expected. My point is simply that we need to be aware of that. Incidentally, I'm only commenting to give some context/rationale behind my previous issue on this.

I did actually understand your point! And it's very good to consider this for any issue. I was trying to point out that #36809 had selection bias as well, that's all.

I now agree with your point that the least common denominator of a bare import path is almost certainly the "best" option if we only have a single copy UI/button/whatever, because it achieves exactly the goal of satisfying the greatest number of people with minimal impact on their workflow.

That said, I also agree with this as a proposed way forward: ...

Great, thank you!

Ideally @julieqiu & team can consider this as part of their broad, ongoing review.

@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented Apr 20, 2020

My 10c is there's almost no world where import "xxx" is useful, if I'm adding it to code then it'll always always be inside an import ( ... ) block or like OP mentioned, using it for go get.

I usually end up just highlighting it and ctrl+ins it rather than using the button.

@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Apr 21, 2020

I think the best solution here is to devise a header that enables:

  • Viewing the package path
  • Copying the package path to clipboard as the default or more prominent option
  • Viewing the import statement (possibly with the actual package name as I mentioned)
  • Copying the import statement (ideally this can be done without a bunch of additional clicks or modal popups)

I agree that this would be the best way forward, and we are planning to add this feature in an upcoming round of design updates.

In the meantime, it seems like having the copy button only copy the import path is the least surprising behavior, so we can revert #36809.

/cc @fflewddur @Joanne881107 @jba

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 21, 2020

@tooolbox

I did actually understand your point! And it's very good to consider this for any issue. I was trying to point out that #36809 had selection bias as well, that's all.

Ah I see. Indeed, #36809 did have a selection bias, totally agree. But the basis for the change was to make pkg.go.dev more like godoc.org, but with the addition of the copy button. The one thing that was missed (as was pointed out above) in that change was that the copied value differed from the text shown, which is a jarring UI for sure. Notwithstanding that point, I think we're headed in the right direction now, thanks @julieqiu!

@julieqiu julieqiu changed the title go.dev: "Copy path to clipboard" actually copies an import statement pkg.go.dev: "Copy path to clipboard" actually copies an import statement Apr 23, 2020
@julieqiu julieqiu changed the title pkg.go.dev: "Copy path to clipboard" actually copies an import statement pkg.go.dev: provide multiple "copy to clipboard" options Apr 23, 2020
@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Apr 23, 2020

In the meantime, it seems like having the copy button only copy the import path is the least surprising behavior, so we can revert #36809.

This is now live.

gopherbot pushed a commit to golang/pkgsite that referenced this issue Jun 15, 2020
The copy button at the top of a details page now puts just the path
on the clipboard, rather than

   import "path"

Updates golang/go#36809.
Updates golang/go#38514.

Change-Id: I7f475d0415f67a4ff72e95abef750d2604717a51
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/722967
Reviewed-by: Julie Qiu <julieqiu@google.com>
@julieqiu julieqiu changed the title pkg.go.dev: provide multiple "copy to clipboard" options x/pkgsite: provide multiple "copy to clipboard" options Jun 15, 2020
@julieqiu julieqiu removed this from the Unreleased milestone Jul 31, 2020
@julieqiu julieqiu added this to the pkgsite/design-2020 milestone Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.