Tips: Add interactivity #4155

pjhampton · 2017-05-03T21:05:15Z

Description of new Instant Answer, or changes

Adds interactivity to tips. Good for review 👍

Related Issues and Discussions

People to notify

@moollaza

Testing & Review

To be completed by Community Leader (or DDG Staff) when reviewing Pull Request

Pull Request

Title follows correct format (Specifies Instant Answer + Purpose)
Description contains a valid Instant Answer Page Link (e.g. https://duck.co/ia/view/my_ia)

Instant Answer Page (for new Instant Answers)

Instant Answer page is correctly filled out and contains:
- The Title is appropriately named and formatted
- The IA topics are present and appropriate
- The Description is clear and coherent
- Source Name exists if applicable
- All Example Queries trigger on Beta

Code

Adheres to the DuckDuckGo Style Guide
Behaviour is appropriately tested. If improvement, tests are adequately extended.
There is no unnecessary files in place (such as editor config files)
There is no API keys / secrets present
Tests are passing (run $ duckpan test <goodie_id>)
- Tester should report any failures

Ready to merge?

Has this IA been deployed to and tested on beta.duckduckgo.com?
For larger commits, has this been approved be more than one community member?
The number of reviews is appropriate for this type of PR
The commit message is clear, coherent and fitting

Pull Request Review Guidelines: https://docs.duckduckhack.com/programming-mission/pr-review.html

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

daxtheduck · 2017-05-03T21:05:15Z

Tips

Description: calculate a total including a percentage tip

Example Query: 20% tip on $21.63, 20 percent tip for a $20 bill

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

moollaza · 2017-05-05T21:58:25Z

Deploying to Beta 👍

pjhampton · 2017-05-08T20:50:26Z

@duckduckgo/duckduckhack-contributors Does anybody want to test drive this / code review? it would be greatly appreciated!

mattr555

Just a few nitpicks 👍 🚀

mattr555 · 2017-05-11T23:59:12Z

share/goodie/tips/tips.css

+
+.zci--tips span#tip_label,
+.zci--tips span#total_label {
+    font-size: 1.5em


Could we decrease the bottom-padding on the labels to 0 so it's more clear which number belongs to which label?

(You'll also have to change span to h4 in this rule.)

Yeah, I see what you're saying. I believe @moollaza reached out to the internal design team to run their eyes over it. Once I have that feedback I will incorporate all your changes. Thanks for the feedback @mattr555. Super appreciated!

mattr555 · 2017-05-12T00:09:07Z

share/goodie/tips/tips.js

+                     * the input buttons
+                     */
+                    $bill_input.change(function(_e) {
+                        calculateTip()


I noticed when using the input buttons, it rounds off the value in the "Bill" field:

This isn't super important, but it would be nice if it avoided this and rounded off when using these buttons.

Oh yeah, that is annoying. Well spotted. Will fix 👍

mattr555 · 2017-05-12T00:10:38Z

share/goodie/tips/tips.js

+
+        return {
+            onShow: function() {
+                DDG.require('math.js', function() {


Any reason why this dependency is here?

Nope! I was planning to use it but just used plain js in the end. Will fix 👍

pjhampton · 2017-05-15T17:14:50Z

I think this is good to be shipped. I have done some sec testing as well. All seems ok 👍

pjhampton · 2017-05-16T11:03:19Z

moollaza · 2017-05-22T01:42:17Z

share/goodie/tips/tips.js

+                 * Event handlers to update the values when
+                 * keys are pressed
+                 */
+                $bill_input.keyup(function(_e) {


We should grab all the inputs with a jQuery selector and apply a single keyUp event handler

$inputs = $dom.find('input'); // use change to handle both keyup and click $inputs.change(function(){ normalizeText(); calculateTip() });

moollaza · 2017-05-22T01:43:59Z

share/goodie/tips/tips.js

+                    setUpSelectors();
+                    $bill_input.val(init_bill);
+                    $bill_tip.val(init_percentage);
+                    calculateTip()


We need to start doing this pre-render in interactive IAs to prevent lag. It's noticeable on slower computers and slower internet connections

Yeah, I agree. Maybe a unified approach to this might be a good starting point.

moollaza · 2017-05-22T01:52:45Z

share/goodie/tips/tips.js

+
+        var tip = bill_input * (bill_tip / 100);
+        var tip_pp = parseFloat(tip) / parseInt(bill_people);
+        var total = parseFloat(bill_input) + parseFloat(tip);


you don't need the parseFloat and parseInt here. Javascript will figure it out :)

You could also do things like (intA + floatB).toFixed(2)

It seems that although inputs are type='number' they jquery still fetches them as string objects that need to be parsed as floats/ints

moollaza · 2017-05-22T01:59:49Z

share/goodie/tips/tips.css

+    font-size: 1.5em
+}
+
+.zci--tips div#tips_block_tip,


No need to use ID's here -- this is a perfect use case for selectors or classes:

/* with a class on parent div */ .zci--tips frm--right div { ... } /* OR */ /* with a class on the divs we're targetting */ .zci--tips tips_block { ... } /* OR */ /* approach without IDs */ .zci--tips tips_block--tip, .zci--tips tips_block--total, { ... }

I use these ids for the responsive layout now 👍

You can still use the IDs on the elements, but I would add a class to both to simplify the CSS.

internally we use a js-foo class to specify things we'll target with JS. We typically avoid using ids:

<div class="tips_block tips_block--tip js-tips_block-tips">

The first class can be used for CSS rules like the one above, the 2nd for element-specific CSS, the third for a JS handler

moollaza · 2017-05-22T02:38:53Z

share/goodie/tips/tips.css

+    font-weight: bold;
+    text-align: center;
+    margin-bottom: 10px;
+    width: 90%;


If you use class="ninety" on the element this will force 100% width on mobile

This seemed to cause a lot of problems believe it with the mobile and desktop design

pjhampton · 2017-05-26T09:23:11Z

Does anybody have any last minute feedback? @duckduckgo/duckduckhack-contributors

pjhampton · 2017-06-15T21:39:41Z

@duckduckgo/community-leaders seems good to go 👍

adityatandon007 · 2017-06-16T04:04:00Z

@pjhampton It doesn't look good to go as far as I am concerned.

Solution: You deleted the click function in js so I think you need to add it to support the arrow buttons.

pjhampton · 2017-06-16T09:27:11Z

~~Seems like an edge case. What browser is this @adityatandon007? I can't replicate.~~

edit: Nvm. I see @adityatandn007

moollaza · 2017-06-23T21:12:49Z

@pjhampton Thanks! This LGTM 👍

pjhampton added Improvement Status: Needs Community Review Status: Work In Progress labels May 3, 2017

pjhampton assigned moollaza May 3, 2017

pjhampton requested review from moollaza and gaulrobe May 3, 2017 21:05

daxtheduck assigned gaulrobe and unassigned moollaza May 3, 2017

pjhampton removed the Status: Work In Progress label May 4, 2017

pjhampton changed the title ~~Tip: Add interactivity~~ Tips: Add interactivity May 5, 2017

moollaza added the Design label May 5, 2017

daxtheduck deployed to beta.duckduckgo.com May 5, 2017 21:58 Active

mattr555 reviewed May 12, 2017

View reviewed changes

moollaza suggested changes May 22, 2017

View reviewed changes

daxtheduck deployed to beta.duckduckgo.com May 25, 2017 16:00 Active

daxtheduck deployed to beta.duckduckgo.com May 25, 2017 16:37 Active

daxtheduck deployed to beta.duckduckgo.com May 26, 2017 16:54 Active

sahildua2305 approved these changes May 31, 2017

View reviewed changes

moollaza approved these changes Jun 5, 2017

View reviewed changes

pjhampton added 2 commits June 16, 2017 10:49

initial commit.

9e69d89

Fixed NaN issue with bill amount.

75bd9f9

pjhampton and others added 12 commits June 16, 2017 10:49

Finish up tips.

bd10804

Fixed markup.

d0f825e

Remove mathjs as dep

0ab3240

Finishing touches...

9d9a3c6

Tweak design.

fc0ca92

Add class

075df7f

Add simplified css

4217397

Add total per person.

5b36e4a

Add secondary text

323b872

Update input class.

6669f21

overhaul ui

fce1529

Add further bindings.

2eaf4b6

pjhampton force-pushed the pjhampton/tips-add_interactivity branch from 1708fc8 to 2eaf4b6 Compare June 16, 2017 09:50

Removed rouge file.

03abd33

daxtheduck deployed to beta.duckduckgo.com June 23, 2017 20:43 Active

moollaza merged commit ee9d459 into master Jun 23, 2017

moollaza deleted the pjhampton/tips-add_interactivity branch June 23, 2017 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tips: Add interactivity #4155

Tips: Add interactivity #4155

pjhampton commented May 3, 2017 •

edited

daxtheduck commented May 3, 2017 •

edited

moollaza commented May 5, 2017

pjhampton commented May 8, 2017

mattr555 left a comment

mattr555 May 11, 2017

pjhampton May 12, 2017

mattr555 May 12, 2017

pjhampton May 12, 2017

mattr555 May 12, 2017

pjhampton May 12, 2017

pjhampton commented May 15, 2017

pjhampton commented May 16, 2017

moollaza May 22, 2017

moollaza May 22, 2017

pjhampton May 24, 2017

moollaza May 22, 2017

pjhampton May 24, 2017

moollaza May 22, 2017

pjhampton May 24, 2017

moollaza May 24, 2017

moollaza May 22, 2017

pjhampton May 24, 2017

pjhampton commented May 26, 2017

pjhampton commented Jun 15, 2017

adityatandon007 commented Jun 16, 2017 •

edited

pjhampton commented Jun 16, 2017 •

edited

moollaza commented Jun 23, 2017

Tips: Add interactivity #4155

Tips: Add interactivity #4155

Conversation

pjhampton commented May 3, 2017 • edited

Description of new Instant Answer, or changes

Related Issues and Discussions

People to notify

Testing & Review

daxtheduck commented May 3, 2017 • edited

moollaza commented May 5, 2017

pjhampton commented May 8, 2017

mattr555 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjhampton commented May 15, 2017

pjhampton commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjhampton commented May 26, 2017

pjhampton commented Jun 15, 2017

adityatandon007 commented Jun 16, 2017 • edited

pjhampton commented Jun 16, 2017 • edited

moollaza commented Jun 23, 2017

pjhampton commented May 3, 2017 •

edited

daxtheduck commented May 3, 2017 •

edited

adityatandon007 commented Jun 16, 2017 •

edited

pjhampton commented Jun 16, 2017 •

edited