Refactoring whois spice #1341
Refactoring whois spice #1341
Conversation
Changes: - Provided explicit template for whois info table rather than inheriting template from downstream code outside of this repo. - More semantic HTML: - For available domains: introduced <p> and <span> tags instead of inline text nodes and <br> tags and removed duplicate <strong> tag. - For taken domains: refactored table into a more semantic definition list for key/value pairs. - Made code formatting consistent (now only follows one style for things like spacing, quotes, etc) - Refactored JS to use function statements rather than function expressions where possible. This has the following benefits: - This is the most readable way to obey the [onevar rule](http://wonko.com/post/try-to-use-one-var-statement-per-scope-in-ja vascript). - This will give you a named function in a stack trace in your debugger rather than an anonymous function, which makes debugging easier. - There is a [small performance advantage](http://jsperf.com/fn-expression-vs-statement). - Added [Oxford comma](http://thumbnails-visually.netdna-ssl.com/the-oxford-comma_52c855 ed979ed_w1500.jpg) to register domain links.
|
These changes look great 👍 One note, I believe the Current: After PR #1341 |
|
@kethinov Thanks Eric. The semantic markup piece is interesting. Curious what your thoughts are on that vs. performance? We've been forcing a lot of the spices into the same templates to reuse markup/styles + limit the KB sent to the client. |
|
@bsstoner I don't think network performance is significantly different with these changes. The markup is significantly more concise but the CSS is also a bit more verbose. Both are cached anyway after the first search. I think the main benefit here is more maintainable code. There is a slight improvement in client-side JS performance which comes from using function statements rather than function expressions. There may also be a slight client-side JS performance advantage from handlebars having to parse less markup, but I don't think it's anything to write home about. @chrisjwilsoncom I fixed the issue you found, thanks. 😄 |
|
@kethinov thanks a lot for this! I like all the changes, but I'm not sure about a custom template here. With regards to performance, all our Handlebars templates are precompiled when we deploy. We really prefer the use of templates so we can maintain uniformity with regards to design and hopefully eliminate the need for any custom templates and css -- this one is a bit of an exception to that rule. I'd rather work on improving the templates and the templating system though, so the benefits are passed on to other IAs too. Unfortunately, the templates aren't open source yet, but it's definitely something we plan to do. |
|
Ah I see, I just poked around the minified code here https://duckduckgo.com/g1214.js and I see now that the internal "record" template from downstream is being served to all requests. Given that, adding a custom template for a spice would indeed add some page weight, so I've reverted to the "record" template for now. I also tweaked the code to add the alternating row highlight to match the visual style of other spices that use this template. |
|
Here are a couple screenshots:
|
|
@kethinov thanks a lot, this is looking awesome! I'll be sure to test and merge shortly. |
| meta: { | ||
| sourceName: "Whois API", | ||
| sourceName: 'Whois API', | ||
| sourceUrl: 'https://www.whoisxmlapi.com/#whoisserver/WhoisService?domainName=' | ||
| + api_result.domainName | ||
| + '&outputFormat=json&target=raw' | ||
| }, | ||
| templates: { | ||
| group: 'base', |
There was a problem hiding this comment.
Let's use the text template, that way we can provide a title. It also allows the "More at" to show properly, it wasn't working for me when I tested this PR.
I think setting the title to the domain of the result would be good. Looks like this:
There was a problem hiding this comment.
Adding a subtitle to make it 100% clear what you're looking at also looks quite nice:
@chrismorast @abeyang does this look good?
|
Whoops, was just about to submit the changes. |
|
You're right though, there should probably only be one PR open for this right now. Looks like @chrisjwilsoncom's will need to be rebased, but otherwise that'll be cleaner. |
|
@kethinov Will be rebasing shortly! Thanks for your contributions they've encouraged me to better refactor my code 😄 |
|
👍 |
|
Hi, instead of the zebra stripes and the title/subtitle combo, can we have it more like this:
Also note that I have the date/time set to the local timezone. Thanks! |
Changes:
<p>and<span>tags instead of inline text nodes and<br>tags and removed duplicate<strong>tag.For taken domains: refactored table into a more semantic definition list for key/value pairs.