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.

Sign up for GitHub

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

Added support for various endpoints to Paper Badget widget. #131

Merged
merged 3 commits into from
Sep 28, 2015
Merged

Added support for various endpoints to Paper Badget widget. #131

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
877216c
IE9 bug fix to ensure site furniture shows correctly.
Sep 11, 2015
7008693
Merge branch 'master' of https://github.com/mozillascience/PaperBadger
Sep 24, 2015
cfc78e9
Paper Badger widget with added support for different endpoints.
Sep 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 30 additions & 8 deletions public/widgets/paper-badger-widget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,44 @@ function splitArray(arr, lookGroup, taxonomyClass)
return ttt;
}

function getEndpoint(confIn, count)
Copy link
Member

Choose a reason for hiding this comment

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

Code wise this works. Style wise I find having both + and join('/') confusing:

    function getEndpoint(confIn, count) {
        var endPoint=[];
        endPoint.push("https://badges.mozillascience.org");
        if (confIn["article-doi"]) endPoint.push("papers/" + confIn["article-doi"]);
        if (confIn["orcid"]) endPoint.push("users/" + confIn["orcid"]);
        endPoint.push("/badges");
        if (count) endPoint.push("/count" + count)
        return endPoint.join("/");
    }

    function getEndpoint(confIn, count) {
        var endPoint = "https://badges.mozillascience.org";
        if (confIn["article-doi"]) endPoint += "papers/" + confIn["article-doi"];
        if (confIn["orcid"]) endPoint += "users/" + confIn["orcid"];
        endPoint += "/badges";
        if (count) endPoint += "/count" + count;
        return endPoint;
    }

Would either of those be clearer? I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Rob,

Please don’t look at this from a semantic perspective.

Every function was chosen carefully and because of the function they provided.

“Push” in this case, ensures the correct order is maintained for every part of each possible endpoint supported.

“Join” collects and renders each part of the composition, without a need for loop and ensures they are separated by “/” – all this encapsulated in a single function, simple and elegant – keeping the code clean, free from confusing clutter.

And the last part acts as a test, so “/count” is only added as a suffix when required.

Kindest regards,

Al.

From: Robert Peters [mailto:[email protected]]
Sent: 24 September 2015 19:42
To: mozillascience/PaperBadger
Cc: Coleman, Alistair
Subject: Re: [PaperBadger] Added support for various endpoints to Paper Badget widget. (#131)

In public/widgets/paper-badger-widget.jshttps://github.com//pull/131#discussion_r40356064:

@@ -87,22 +87,44 @@ function splitArray(arr, lookGroup, taxonomyClass)

  return ttt;

}

+function getEndpoint(confIn, count)

Code wise this works. Style wise I find having both + and join('/') confusing:

function getEndpoint(confIn, count) {

    var endPoint=[];

    endPoint.push("https://badges.mozillascience.org");

    if (confIn["article-doi"]) endPoint.push("papers/" + confIn["article-doi"]);

    if (confIn["orcid"]) endPoint.push("users/" + confIn["orcid"]);

    endPoint.push("/badges");

    if (count) endPoint.push("/count" + count)

    return endPoint.join("/");

}



function getEndpoint(confIn, count) {

    var endPoint = "https://badges.mozillascience.org";

    if (confIn["article-doi"]) endPoint += "papers/" + confIn["article-doi"];

    if (confIn["orcid"]) endPoint += "users/" + confIn["orcid"];

    endPoint += "/badges";

    if (count) endPoint += "/count" + count;

    return endPoint;

}

Would either of those be clearer? I'm not sure.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/131/files#r40356064.

Copy link
Member

Choose a reason for hiding this comment

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

Hi both!

I'm going to r+ and merge this since it works and it's a great feature.

However, I would prefer clearer semantics help onboard new contributors (clever semantics can make it hard for new ppl to join in 😄)

{
var orcid=confIn["orcid"];
var doi=confIn["article-doi"];
var countPoint=(count) ? "/count" : "";

var endPoint=[];
if(doi) {endPoint.push("papers/"+doi);}
if(orcid) {endPoint.push("users/"+orcid);}
return "https://badges.mozillascience.org/"+endPoint.join("/")+"/badges"+countPoint;
}

function showBadgeFurniture(confIn)
{
var furnitureClass=(confIn["furniture-class"]) ? confIn["furniture-class"] : "paper-badges-hidden";
var doi=confIn["article-doi"];
var endPoint=getEndpoint(confIn, 1);

callAjax("https://badges.mozillascience.org/papers/"+doi+"/badges/count", function(dataItem){
callAjax(endPoint, function(dataItem){
var badgesCount=(dataItem) ? dataItem : 0;

if(badgesCount && dataItem/dataItem)
{
visitArray(document.querySelectorAll("."+furnitureClass), function(elem){

if(elem.classList.contains(furnitureClass))
var list=elem.classList;

if(list!==undefined && list.contains(furnitureClass))
{
elem.classList.remove(furnitureClass);
list.remove(furnitureClass);
}
else
{
var className=elem.className;
var indStart=className.indexOf(furnitureClass);
var indEnd=className.indexOf(" ", indStart);

indEnd=(indEnd>=0) ? indEnd : className.length;
elem.className=className.substring(0, indStart)+" "+className.substring(indEnd);
}
});
}
});
Expand All @@ -113,14 +135,14 @@ function showBadges(confIn, callback){
var k="";

var containerClass=(confIn["container-class"]) ? confIn["container-class"] : "badge-container";
var doi=confIn["article-doi"];
var endPoint=getEndpoint(confIn);

if(callback)
{
clickEvent.assignCallback(callback);
}

callAjax("https://badges.mozillascience.org/papers/"+doi+"/badges", function( entryData ) {
callAjax(endPoint, function( entryData ) {

var outCount=0;
var pos=0, end=0;
Expand Down