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

Export citation popup #1375

Merged
merged 20 commits into from
Jun 25, 2021
Merged

Export citation popup #1375

merged 20 commits into from
Jun 25, 2021

Conversation

mbollmann
Copy link
Member

@mbollmann mbollmann commented Jun 14, 2021

This implements a feature discussed in #412 and supersedes #1095. (this now does none of these things)

This implements a new citation popup modal, making the interface nicer and giving us more options for hopefully providing more citation options soon (re #412).

Feedback welcome!

@github-actions
Copy link

Build successful. You can preview it here: https://preview.aclanthology.org/export-citation-popup
This preview will be removed when the branch is merged.

@mbollmann mbollmann requested review from mjpost and a team June 14, 2021 16:29
@mjpost
Copy link
Member

mjpost commented Jun 14, 2021

This is fantastic. Comments:

  • In the markdown text, there is a small gap between the pieces of the link: [] ()
  • Copying currently requires three clicks: Cite → [tab] → Copy to clipboard. What do you think about retaining this (for discoverability), but also adding four tiny buttons beneath the "Cite" button (on the main paper page) that does each of them directly? That way, after the first time, users would just learn which to click. There are probably suitable icons for these four.
  • Should the "Bib export formats" line below copy this functionality? It seems odd that we repeat just the "Copy BibTeX to clipboard" line

@mbollmann
Copy link
Member Author

  • I think you're seeing the space before the (author, venue) part; there's no gap within the markdown link for me.
  • What about changing the "Bib export formats" line to have "copy to clipboard" options for each, as a quick access option for these? That way users would only have to open the "cite" dialog if they wanted the file download, or a preview of the texts before copying.
  • Also, there's no plain text / rich text version (as originally requested in Adding a plain ACL-style bibliography entry to the meta-information page.  #412 ...) yet. I'm not sure if this should be another tab in the dialog box or a line below the abstract, as suggested here. What do you think?

@mjpost
Copy link
Member

mjpost commented Jun 14, 2021

  1. Oh, you're right!
  2. I like it.
  3. Could it be both or will that get too crowded?

Very excited about this change.

@mjpost
Copy link
Member

mjpost commented Jun 14, 2021

A further thought: on (3), I like the idea of the inline "Short citation" text, maybe with its own "copy to clipboard" button. We only need to list it once, as rich text; the user can then use system options to paste as rich text or plain text ("paste and match style" on a mac).

@mjpost
Copy link
Member

mjpost commented Jun 15, 2021

I'll add, that if the feedback complicates this PR, such that it gets stalled, let's just postpone them. This is a fantastic improvement and I'd love to see it get pushed out this week!

@mbollmann
Copy link
Member Author

Give me a day though to at least add some kind of plain text citation ;)

@mbollmann mbollmann mentioned this pull request Jun 18, 2021
@mbollmann
Copy link
Member Author

grafik

grafik

@akoehn
Copy link
Member

akoehn commented Jun 18, 2021

Looks good, I just would switch markdown and bibtex, as bibtex is probably the dominant export format.

@mjpost
Copy link
Member

mjpost commented Jun 18, 2021

Increasingly picky comments that can be ignored.

  • The “Cite” button at the top stands out from “PDF” and “Search” in a negative way, to my eye. Not sure why it looks different
  • “Citation (short)” feels wordy to me, and also redundant with the “Copy Citation” block above it. I’d suggest just calling it “Citation”, and moving it just above the “Copy Citation” block
  • We could also also add “RTF” to the list of export options for completeness?

@mbollmann
Copy link
Member Author

* The “Cite” button at the top stands out from “PDF” and “Search” in a negative way, to my eye. Not sure why it looks different

Is it just an aesthetic thing or does it actually look different for you? If the former, might be related to the width of the icon compared to the text. Maybe we could also use something else here now that the button doesn't actually download a file.

This here would be more in line with the citation symbol other websites (e.g. Google/Semantic Scholar) use:
button with quotation mark icon

Or this one:
button with pencil icon

Could of course also make the citation button primary-colored so that it stands out more:
button with primary color

* “Citation (short)” feels wordy to me, and also redundant with the “Copy Citation” block above it. I’d suggest just calling it “Citation”, and moving it just above the “Copy Citation” block

I'm thinking this is a very short and informal format, suitable for an email or blog post, but I wouldn't want to suggest that this is how we encourage papers in the Anthology to be cited. "short" would therefore be in contrast to the longer, ACL-style citation string that we can hopefully also add sometime soon.

* We could also also add “RTF” to the list of export options for completeness?

The button row, or the popup? Would it do the same thing the small copy button after "citation (short)" does right now?

@mjpost
Copy link
Member

mjpost commented Jun 18, 2021

It looks quite different:

image

I like the cite with the double quotes! Probably we should color it as you displayed.

How about "Short citation" instead of "Citation (short)"?

I was thinking the popup and the button row, but I think we can leave it as you have it.

@mbollmann
Copy link
Member Author

tl;dr: Adding preformatted citations is not free of problems. Do you feel it's worth it to proceed with this?

I've tweaked this a bit more so you can see what the code looks like & what this produces, currently only in the "Cite" popup, for example:

grafik

Some observations:

  • The overhead for instantiating the Anthology class was indeed significant, from 27 seconds (without the citeproc code) to 193 seconds for me. I've moved the citation string generation to create_hugo_yaml.py now, but it'll still have an effect on the full build time of course.
  • I can't find a CSL style that uses the short title fields for the proceedings (e.g. "EACL" instead of "Proceedings of the ..."). I think Semantic Scholar only does this because they don't even have the full name of the proceedings in their bib info. If we want something like this, I think we'd have to construct a variant where we substitute the short conference title explicitly.
  • None of the other styles I've considered (Chicago, APA, MLA) appear to make use of the "month" field for conference paper entries.
  • The CSL styles are not free of problems:
    • ACL style has problems with "month" field (as reported before)
    • Chicago styles crash on page ranges with Roman numerals, so I left it out for now
    • MLA style with URL produces problematic HTML (unescaped angle brackets), so I used the version without the URL for now
    • Citeproc-py's HTML formatter does not add hyperlinks to the output, neither for titles nor for URLs

I'd love to hear your thoughts on whether it's worth it to proceed with this. I could also move the more controversial/diificult changes to a separate PR, so we could proceed with merging the new citation popup already.

@nschneid
Copy link
Contributor

Seems to me like having multiple citation styles (MLA, Chicago, APA) is overkill—I don't expect the Anthology gets a ton of users who would use that (though who knows, I could be wrong).

The plain text ACL seems like a nice-to-have for putting together informal bibliographies (say, in a blog post or course document) that are not in LaTeX. A warning message (beta feature—may contain errors) could excuse having occasional errors in these.

@nschneid
Copy link
Contributor

Short conference names are a separate issue (#567, #791). Would be great to have eventually, but I wouldn't hold up this feature over that.

@nschneid
Copy link
Contributor

  • ACL style has problems with "month" field (as reported before)

Have you tried the CSL fix I suggested above?

@akoehn
Copy link
Member

akoehn commented Jun 23, 2021

I would propose we table the plaintext references for now and merge the easy (i.e. sone) parts.

Maybe we can find js bibtex renderer at some point in time and can mitigate the whole up-front generation.

@mjpost
Copy link
Member

mjpost commented Jun 23, 2021

I like having an RTF citations, but agree we don't need all of them. I don't share David's concern about the text format, but would rather have consensus here before moving forward on this. Moreover, I'm working about increasing the instantiation time, since it really make debugging a pain. Perhaps we should fix #835 first.

So how about we factor the controversial stuff into another draft PR, and at least get the new popups live?

@mbollmann
Copy link
Member Author

I agree that plain-text/rich-text ACL-style format would be the nicest. If that's all we need, maybe we can also build our own formatter for that in pure Python, to reduce overhead.

Short conference names are a separate issue (#567, #791).

Yes, but tangential to this, as the CSL style files won't use them for anything.

Have you tried the CSL fix I suggested above?

Not yet! We should, but only if we go forward with having citeproc-py in the pipeline.

Moreover, I'm working about increasing the instantiation time, since it really make debugging a pain.

Instantiation time shouldn't be increased anymore with the latest commit here; overall build time is.

@mbollmann mbollmann changed the title Export citation popup (implements #412) Export citation popup Jun 23, 2021
@mbollmann mbollmann marked this pull request as ready for review June 23, 2021 22:13
@mbollmann mbollmann requested review from a team and mjpost June 23, 2021 22:13
@mbollmann
Copy link
Member Author

➡️ Split off into #1390

@mjpost
Copy link
Member

mjpost commented Jun 24, 2021

Oh, I thought we'd at least be keeping Markdown in this PR. Any objections to that? My fear is that #1390 is going to get stalled...

I'll lave the squash-merge to you, Marcel!

@mbollmann
Copy link
Member Author

Oh, I thought we'd at least be keeping Markdown in this PR. Any objections to that?

But isn't that exactly what David and others were concerned about?

@mjpost
Copy link
Member

mjpost commented Jun 24, 2021

I thought David's concern stemmed from having a plain text "Short citation" format, inline on the main page. The Markdown format seems different to me (though maybe it's not in his mind).

@mbollmann
Copy link
Member Author

I thought David's concern stemmed from having a plain text "Short citation" format, inline on the main page. The Markdown format seems different to me (though maybe it's not in his mind).

It's exactly the same format, though? The "short citation" was just the rendered Markdown string. Of course it's less prominent if it only appears in the popup, and I could also include a footnote there that it's not an official citation format... I dunno, I'll go with whatever you (plural) feel is fine and not too problematic.

@mjpost
Copy link
Member

mjpost commented Jun 24, 2021

The risk of cutting-and-pasting seems even lower to me (less visible, requires adjusting), but probably we should leave things as they are.

@davidweichiang
Copy link
Collaborator

Sorry, I hadn't looked at the Markdown before and would have the same opinion about the "short" citation in any format.

On my browser (Safari 14) the preview looks like this:

image

which is a little weird.

@mbollmann
Copy link
Member Author

Maybe the Bootstrap version we use doesn't style type="button" correctly on Safari ... since I don't have an iOS device, could you check again now @davidweichiang?

@davidweichiang
Copy link
Collaborator

Yup, after a refresh that looks great.

@mjpost
Copy link
Member

mjpost commented Jun 24, 2021

Same here.

@mbollmann mbollmann merged commit dd57942 into master Jun 25, 2021
@mbollmann mbollmann deleted the export-citation-popup branch June 25, 2021 12:49
@nschneid
Copy link
Contributor

Beautiful! Announce the new feature on Twitter?

@davidweichiang
Copy link
Collaborator

Looks great.

Tangential question: What does the Search button do? Just searches on the title?

@mbollmann
Copy link
Member Author

Tangential question: What does the Search button do? Just searches on the title?

Yes exactly; on desktop the mouseover text will also say so. Opinions on whether or not to replace Google Scholar with Semantic Scholar via #1380, for the reasons expressed in #1339, are currently welcome!

@mjpost mjpost mentioned this pull request Aug 19, 2021
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.

6 participants