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

Add new JSMinify Instant Answer #3352

Merged
merged 22 commits into from Jul 19, 2016

Conversation

Projects
None yet
5 participants
@sahildua2305
Copy link
Member

sahildua2305 commented Jul 5, 2016

What does your Pull Request do (check all that apply)?

Choose the most relevant items and use the following title template to name
your Pull Request.

  • New Instant Answer
    • Cheat Sheets: New {Cheat Sheet Name} Cheat Sheet
    • Other: New JS Minify Instant Answer
  • Improvement
    • Bug fix: {IA Name}: Fix {Issue number or one-line description}
    • Enhancement: {IA Name}: {Description of Improvements}
  • Non–Instant Answer
    • Other (Role, Template, Test, Documentation, etc.): {GoodieRole/Templates/Tests/Docs}: {Short Description}
Description of changes

Developing an interactive JS Minifier using this library. Work in progress

People to notify (@mention interested parties)

@moollaza @MariagraziaAlastra


Instant Answer Page: https://duck.co/ia/view/js_minify

@daxtheduck

This comment has been minimized.

Copy link

daxtheduck commented Jul 5, 2016

Js Minify

Description: An Interactive JavaScript Minifier Tool

Example Query: [js minify](https://beta.duckduckgo.com/?q=js minify), [js minifier](https://beta.duckduckgo.com/?q=js minifier)

Tab Name: Answer

Source:

These are the important fields from the IA page. Please check these for errors or missing information and update the IA page


This is an automated message which will be updated as changes are made to the IA page

zci answer_type => 'jsminify';
zci is_cached => 1;

triggers any => 'js minify', 'js minifier';

This comment has been minimized.

@MariagraziaAlastra

MariagraziaAlastra Jul 5, 2016

Member

@sahildua2305 I think we'd also want this to trigger for queries like "javascript minify, javascript minifier, minify js, minify javascript"

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 5, 2016

Author Member

Definitely! This is just the beginning. I have added these trigger words for testing. I'll add more. Thanks for suggesting these words. 😄

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 5, 2016

@sahildua2305 Thanks for working on this! Please let me know if you need any help 😄

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 5, 2016

@MariagraziaAlastra Actually I need help in getting started with this. With this initial setup, I am not able to show anything on search page. The IA gets triggered but doesn't show anything. Not even an IA tab.

Can you please look into this? I may be missing something naive.

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 5, 2016

@sahildua2305 It actually triggers for me:
screenshot-devour-sertum codio io 5000 2016-07-05 22-55-58

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 5, 2016

Weird! I'll check again. Thanks a lot.

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 6, 2016

@MariagraziaAlastra I have pushed a working interactive js minifier (with very basic UI) and it looks like this -
selection_154

Please deploy this on Beta. 😄

return;

// set the flag to true so it doesn't get run again
shown = true;

This comment has been minimized.

@gaulrobe

gaulrobe Jul 6, 2016

Member

This shown logic seems like it is indicative of a different, underlying problem... does onShow get called multiple times?

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 6, 2016

Author Member

onShow gets called every time the Answer tab is shown.

For example, if user searches for js minify by default Answer tab will be opened. If user clicks on Web tab and then comes back to Answer tab, onShow will be called again.

This comment has been minimized.

@gaulrobe

gaulrobe Jul 6, 2016

Member

I see this is done in other javascript, but it just seems like something that should be built-in at a lower level. Carry on!

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 6, 2016

Author Member

Agreed! 👍

@daxtheduck daxtheduck deployed to beta.duckduckgo.com Jul 6, 2016 Active

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 6, 2016

@sahildua2305 Hey, nice job here, thanks!
Your PR is deployed to beta, but it doesn't trigger yet. Will let you know when we have the library installed on our end, so you can use DDG.require() instead - it shouldn't take long anyway. 😄

I tested it on DuckPAN in the meantime and it works really well. As discussed on Slack, why don't you make the tab name be "Minifier" instead? Maybe "JavaScript Minifier", even.

sahildua2305 added some commits Jul 6, 2016

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 6, 2016

@MariagraziaAlastra Thanks 😇

I have changed the tab name to "Minifier" so that even CSS minifier can come under the same name. However it's not showing up as Minifier in search results.

Also, @moollaza the CSS minifier doesn't need to be different. I can change this IA's name to Minifier and hence it can be used to minify CSS or JS depending upon the query. There can be an option to choose between CSS and JS for the user. What do you think?

@GuiltyDolphin GuiltyDolphin changed the title Add new JS minifier Add new JSMinify Instant Answer Jul 6, 2016

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 15, 2016

@MariagraziaAlastra please deploy this on beta so that we can test it. I have made the library load on change in textarea contents. I have also disabled the button till the time library is loaded.

@daxtheduck daxtheduck deployed to beta.duckduckgo.com Jul 18, 2016 Active

$minifyButton.css('cursor', 'default');

// Add event handler for change in input of textarea
$input.on('input', function() {

This comment has been minimized.

@MariagraziaAlastra

MariagraziaAlastra Jul 18, 2016

Member

@sahildua2305 maybe try on keyup instead?

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 18, 2016

Author Member

I am afraid keyup won't be triggered if user pastes the code using context menu. input will be triggered whenever the input changes, in any way. @MariagraziaAlastra

This comment has been minimized.

@MariagraziaAlastra

MariagraziaAlastra Jul 19, 2016

Member

Ok cool, I was worried about IE9, but turns out this has basic support there, except for deleting, pressing backspace and cutting, and I think we don't care about either of those 😄

if (!libLoaded) {
// Set the flag to make sure the library isn't loaded
// again and again
libLoaded = true;

This comment has been minimized.

@MariagraziaAlastra

MariagraziaAlastra Jul 18, 2016

Member

👍 good one

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 18, 2016

Hey @sahildua2305 it's deployed again! It works, but for some reason the minified code shows underneath the original code, instead of showing on the right:
screenshot-beta duckduckgo com 2016-07-18 17-21-59

I'm using Firefox 47, 1920 × 1080 screen resolution

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 18, 2016

@MariagraziaAlastra this is weird! It works for me. at a lower resolution. Can you please check the dimensions of div with class zci__main inside #zci-js_minify?

Edit: I just tested on 1920 x 1080 resolution using the emulator. It shows well on the side for me. 😕

Edit 2: I figured out what's causing problem. I'll fix this one right now.

Edit 3: Fixed! Please test it. 😄

@daxtheduck daxtheduck deployed to beta.duckduckgo.com Jul 19, 2016 Active

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 19, 2016

@sahildua2305 Thanks, deployed again! Looks good on my laptop, let me just test it on my desktop in a few hours, before the IAs release.
Hopefully it should be ready!

@sahildua2305

This comment has been minimized.

Copy link
Member Author

sahildua2305 commented Jul 19, 2016

@MariagraziaAlastra let me know how it goes 😄

@MariagraziaAlastra

This comment has been minimized.

Copy link
Member

MariagraziaAlastra commented Jul 19, 2016

@sahildua2305 It's ready! Merging now...
Thanks again for the great work here! 😄

@daxtheduck

This comment has been minimized.

Copy link

daxtheduck commented Jul 19, 2016

@sahildua2305 this is now merged! It should be live on DuckDuckGo within one week (or during the next release) and we'll be sure to notify you when it happens.

After your Instant Answer goes live for the World to use, you will be able to see traffic information about your IA as well as receive feedback directly from users.

Please refer to the maintenance guide and docs to keep up with potential changes to your Instant Answer.

Thank you again for your help!

@MariagraziaAlastra MariagraziaAlastra merged commit 479814b into duckduckgo:master Jul 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -0,0 +1,8 @@
<h5>Minify JavaScript Code on the go!</h5>

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

This should be a title property from data -- don't need the content template to render this 👍

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

Let's change the text to "JavaScript Minifier"

$main.toggleClass('c-base');

// Hide output textarea by default
$output.css('display', 'none');

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

We should hide this by adding the is-hidden class to the template instead no need to create it then hide it in the DOM. It should be created as hidden.

$input = $dom.find('#js_minify__input'),
$output = $dom.find('#js_minify__output');

$main.toggleClass('c-base');

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

why are we doing this?

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 19, 2016

Author Member

Honestly, I don't remember why I removed this class. It was creating some CSS related problems.

I will test whether it's required or not, once I have made the suggested CSS changes.

$output.css('display', 'none');

// Disable 'Minify' button by default and change cursor to default
$minifyButton.prop('disabled', true);

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

should be disabled in template, and enabled in JS when necessary


// Disable 'Minify' button by default and change cursor to default
$minifyButton.prop('disabled', true);
$minifyButton.css('cursor', 'default');

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

Use CSS for this too

$minifyButton.text('Loading..');

// load the library
DDG.require('prettydiff', function() {

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

We should use DDG.build_async to load the library, and only once loaded, display the Instant Answer.

This prevents us from having to deal with inputs and button presses before the lib is loaded.

Look at the WhereAmI Goodie to see how to use DDG.build_async

The differenence is that you use a callback function to return your DDH object, instead of returning an object inside your build function

This comment has been minimized.

@sahildua2305

sahildua2305 Jul 19, 2016

Author Member

@moollaza I should skip this, right?

var output = prettydiff(args);

// hide output textarea by default
$output.css('display', 'inline');

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

We should remove the is-hidden class instead

zci answer_type => 'jsminify';
zci is_cached => 1;

triggers any => 'js minify', 'js minifier', 'javascript minify', 'javascript minifier', 'minify js', 'minify javascript', 'minifier js', 'minifier javascript';

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

this needs to be startend -- any is too broad unless we're expecting the query to have leading and trailing words


triggers any => 'js minify', 'js minifier', 'javascript minify', 'javascript minifier', 'minify js', 'minify javascript', 'minifier js', 'minifier javascript';

handle query_lc => sub {

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

if we want the queries that trigger this to only be our triggers, then we need to handle remainder and check that it's empty


.zci--js_minify .js_minify__action {
background: #5DA4DC;
color: #fff;

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

We shouldn't hardcode colors here, otherwise this will break for DDG themes. Use the tx-clr-- and bg-clr-- classes from https://ddg.gg/styleguide to handle the text and background colours. That way they'll adapt to DDG themes

background: #5DA4DC;
color: #fff;
text-decoration: none;
font-size: 1.25em;

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

Use our tx-## class to handle font size. We have a tx--12 I think or t-xs

margin-top: 3.25em;
}

.zci--js_minify .js_minify__action {

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

Actually, all of this can be handled by out btn class


.zci--js_minify #js_minify__input,
.zci--js_minify #js_minify__output {
width: 45%;

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

We will probably need a CSS rule here to handle mobile UI.

I think width: 100% is best so they're always stacked.

Also, we should set font-face: 'monospace'

<textarea name="js_minify__output" id="js_minify__output" rows="20" cols="60" readonly="readonly" onclick="this.focus();this.select()"></textarea><br>
</div>
<div class="js_minify__action_box">
<button class="js_minify__action">Minify</button>

This comment has been minimized.

@moollaza

moollaza Jul 19, 2016

Member

Our btn class can handle the styling here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.