-
Notifications
You must be signed in to change notification settings - Fork 40
[IN-71][EOSF] Add citation picker to citation-widget #331
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
[IN-71][EOSF] Add citation picker to citation-widget #331
Conversation
…r-osf into feature/citation-widget
* Added ember-concurrency for debouncing requests through .restartable() * Debounce logic using ember-concurrency's task, with yield * Styling and usage of the selected style onto template
baylee-d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things.
| <span> {{mla}} </span> | ||
| <div class="f-w-xl m-t-md">Chicago</div> | ||
| <span> {{chicago}} </span> | ||
| <div class=""> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to have an empty class identifier?
| } | ||
| }, | ||
| _citationText: Ember.observer('selectedStyle', function() { | ||
| const citationLink = this.get('node.links.relationships.citation.links.related.href'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is also being set up in didReceiveAttrs(), would it make more sense to make a new variable citationLink: null up above and change the value of it in didReceiveAttrs() if needed? Then you'd only need to make a call to that variable here.
| } | ||
| } | ||
| }, | ||
| _citationText: Ember.observer('selectedStyle', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way you can use a computed here instead of an observer?
baylee-d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just update to the latest develop to fix conflicts and get the changes for the ember-power-select style import.
app/styles/app.scss
Outdated
| @@ -0,0 +1,2 @@ | |||
|
|
|||
| @import "ember-power-select"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took back what I said earlier since there should be a fix for this now. You shouldn't have to do the @import "ember-power-select" with the newest version of ember-osf.
…r-osf into feature/citation-widget
baylee-d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of super small last things, but after that it might be good to go
| "ember-ace": "^1.2.0", | ||
| "ember-bootstrap-datepicker": "^2.0.3", | ||
| "ember-cli-babel": "6.4.1", | ||
| "ember-concurrency": "0.8.12", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to have ember-concurrency twice here in the same section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope! fixed, good catch
| <div> | ||
| <div id="citationList" class="m-b-md"> | ||
| <div class="f-w-xl">APA</div> | ||
| <span>{{apa}} </span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should maybe add a space in front of the APA citation to keep it consistent with the other styles
baylee-d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
alexschiller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I thought I saw merge conflicts on changelog, but I think changing base cleared those up 🤣
Purpose
It would be nice to allow users to retrieve citations other than MLA, chicago and APA, just like projects do, specially since the API, and citeproc-py engine are already in place. This adds that section to the citation-widget currently used in preprints.
Summary of Changes
Add a citation picker to the citation-widget, allowing users to cite their preprints in any of our supported citation styles
Side Effects / Testing Notes
Due to the large amount of citation styles, pressing something usual, like
aand then waiting will make quite a few requests. Doing that a few times will probably trigger some 429 responses.Ticket
https://openscience.atlassian.net/browse/EOSF-883
Reviewer Checklist
testable and includes test(s)(asynchronous)CHANGELOG.md