Improved widget #148
Improved widget #148
Conversation
Cleaner widget code, can display badges for both DOIs and ORCIDs, examples with asynchronous JavaScript for both are in public/widgets/demos
|
Quick glance, this looks great! I'll review once #147 is closed :) |
|
👍 |
Pull readme changes
…ed-widget # Conflicts: # README.md
…er for showBadges() This includes updated demos and a new demo to test if the showBadges compatibility method works correctly
|
I finally found the badges in the GigaScience journal and realised that we can not merge this, yet, as they do not only use the showBadges method but also showBadgeFurniture from the old widget. I'll have a look at what it does tomorrow. |
|
Oh man, thanks for looking at that! |
|
Flagging @Alicole since he knows this widget better than anyone else! @janhohner and I are working to make sure this doesn't break anything live :) |
|
Hi all, The original code was designed with multi-browser support in mind, considering in the publishing world we have authors and researchers who still use IE8 and above. Strange maybe, but true. It took some time to extensively test ever feature that was used by the widget to correctly parse the endpoint JSON source. Moreover we had support for CORS in the callAJAX request. We've now found it's possible to extend this to work with IE8/9 too. Also the widget was design to group all authors for any particular badge. To make it easier to see which badges authors had subscribed, on hovering over their name all others were highlighted (currently in green -- following SIGS' journal theme). On clicking any of these, the page would redirect to author's orcid entry in a separate but targeted browser tab or window. Finally hovering over any author displays their orcid id in a tool tip. Also there were a number of functions, one to render the widget from various endpoints including doi or orcid identifier. Another to ensure we had control over some handles and links to the widget page, around publisher and journal sites. |
|
Hey @Alicole, I'm currently working on an improved version of this pull request, which will do the following:
Native JSON support works in IE9 and above, IE8 has native JSON support in standards mode. In addition, from 12th January 2016 on, IE11 and IE9 (only on Vista) will be the only versions of IE Microsoft supports (https://support.microsoft.com/en-us/gp/microsoft-internet-explorer), so I think we can safely use native JSON - but I'm open for discussions on this point. Are you sure that support for CORS was built into the original widget code? I can just see a standard XMLHttpRequest there. However, I use the same method to get the data from the API, so if it works now, it will not break in the improved version. As for the links, the code I plan to push adds the title-attribute (so ORCIDs will be shown on hover) and opens the links in a new window, so this will mirror the functionality in the original widget, too. |
Adds a showBadgeFurniture compatibility method, adds the showBadgeFurniture functionality to the new widget (removeClass attribute), adds link click callback functionality (as in the original widget - clickCallback attribute) to the new widget, adds a separate documentation file for the widget and all its configuration options
|
Hi @janhohner The design was approved by publishing team at BMC, so any changes would need their buy in. Also the publishers were very keen we didn't use any id's and that the author names show instead. That's why there is a hover over action, which is a nice to have feature, so it doesn't matter if this doesn't work on all devices and browsers. Also the code was designed to work with a series of endpoints provided, and this was tested. I still don't understand your premise for reimplementing the code for widget from scratch. The widget is now being used in production environments, so you need to be mindful that you don't revert any of the existing behaviour. That's why I always vie towards caution, over a nice clean piece of code. The only changes required was to ensure there is a version that works for Orcid site, as far as the publisher sites are concerned the original widget code works fine. |
|
Hi @Alicole, I did not change anything in the widget's appearance, so there's no problem there. If the publishers were so keen on classes instead of ids, I'll change that. I'll change the behaviour back to showing names on ORCIDs, too (patch coming in the next few hours / days) Hover shows the green highlighting and the ORCID (as in the original widget). Could you give me a list of endpoints that are in use? From what I see only the DOI/badges and ORCID/badges endpoints are used, the /count endpoint is only needed for showBadgeFurniture, which I solve with only one request instead of two. I reimplemented the widget because the code was not very readable and not documented at all. The style guide which is enforced for all other JavaScript files was not followed. Additions that should have been simple like #142 were made quite difficult that way. Additionally, in my oppinion, the amount of methods that are put in global context in the original widget should be put into one object which is then called, especially because this script is directly loaded into external web sites where it is impossible to guarantee that there are no name collisions. I appreciate your input to make the new version 100% backwards compatible - I don't want to break things already running. But I have to disagree with you on the caution vs. clean code issue. In my oppinion this should not be a "either / or" decision, as both are equally important and two sides of the same coin. Clean code improves the chances that new extensions to the code do not create breaking changes because it decreases chances that a developers miss something because they did not see it in the chaos. That way, I'm actually not surprised that nobody touched #142. Furthermore, reimplementing the whole widget gives us more options to, for example, give publishers the options to adjust the colors used in the widget. I'm 100% with you, on the fact that this must not break already existing widgets in production but we can't just stick with version 1 forever for fear of breaking something. |
|
The original widget was not meant to be set in stone, in fact it was implemented in an agile fashion. In any case those who come to the party later, usually benefit from the work of those who acted as the pioneers. In essence you have a blue print to work from, which we didn't have. So here is an opportunity to improve the implementation, which we didn't have at the time. Anything that can be achieved now to make the code more generic is fantastic, keep up the good work. We started with the doi endpoint, which was what was required for the journal pages. Also we requested an /count endpoint was added so we could essentially have a has badges function, useful for displaying site furniture. Finally we modified the endpoint code, so that it supported the orcid endpoint too. |
|
@Alicole On the /count endpoint: I agree with you on the basic idea behind that, but I changed that in my implementation, so we don't need 2 HTTP requests. As we need the badge data anyway, I request the badge data. Then, if I get back badges, I do the rendering + what you did once you had the number of badges in showBadgeFurniture. I really like the general idea of showBadgeFurniture, I just thought this might improve the widget's performance (you can find my implementation here: https://github.com/janhohner/PaperBadger/blob/improved-widget/public/widgets/paper-badger-widget.js#L259-L274, I would appreciate your thoughts on how I solved it) Do you know if the widget is used somewhere to display badges for an ORCID? I thought it might make more sense to display the paper titles or the DOI below the badges in that case instead of repeating the author's name x times, but I'm open for discussions on this point, especially if this is already in use like that. |
|
There are two options, i). create a new widget for Orcid -- as the endpoints are generic in any case ii). adapt the original widget to support an alternative view -- this was my precept for extending its usage. As you can see, just going and reimplementing existing code is fine in principle but you may fail to understand why the approach was adopted in the first place. Perhaps even several were tried before the final widget was released. So may be you might think about taking some of your ideas, and adopting them with the original one. That's the point of agile, small incremental improvements -- rather than reimplementing the wheel, where you find that you're just go through the same pain points as we did first time round. In the process you miss the point, if you're not adding value to the initial design or code base, your premise has failed. And it's time to take a step back, and rethink your approach. |
|
At first, my implementation was completely seperate (as you can see in the commits I made), but @acabunoc asked me to replace the original implementation when she saw my code. I actually had started on improving the original before that, until I realised that I would have to touch every line of code, if I wanted the file to comply to Mozilla's code styles. I think it is obvious that I am going to ask the same questions you did and run into the same pains, if there isn't any documentation of the code and no documentation why and which decisions were made in the original implementation. I am actually working on adding as much of the information I get from you to the code, so we don't get the same problem again in the future. I'll change the ORCID widget to deliver the same output the original widget did, but I am going to add a flag that the users can decide whether they want the original behaviour (names) or the one I proposed (DOIs), as you proposed in option II. The flag default will be to show the original variant, so no breaking changes there either. I normally don't join a new project and drop the hammer on existing code like that, but that's the way it went and I can't change that now. I'm sorry if I offended you in any way - this was not my intention. Like I said, I actually wanted to add my code as a seperate widget. |
|
We'll in life there is a line and you have to decide should I step over it, which in this case you did. Personally this approach doesn't make sense, and to me sounds like a complete waste of time. I'm sure you have better use of your expertise, than reimplementing someone else's code. |
|
Hi @Alicole & @janhohner, I think this got out of hand! Sorry again for my absence while I was out of the office. I think it would be better to approach this as two separate widgets, one with the new features. I didn't know the extent of features / support built into the original widget. @Alicole, thanks for documenting some of them here. Can we record these somewhere discoverable to help new contributors come in and understand what's going on? In the meantime, we will build up our test suites to make sure we keep coverage and compatibility moving forward on the new widget. We're working to build a welcoming community around this project -- please don't dismiss others' efforts. Thanks! A massive thank you to both of you for all your hard work. @Alicole, I'd love to see you listed under our contributors. If you don't have any objections, can you add |
|
Moved to #150. |
Cleaner widget code, follows the code style, gets linted with npm run lint. Widget can display badges for both DOIs and ORCIDs, examples with asynchronous JavaScript for both are in public/widgets/demos