Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixed "Estimated Time Saved" to take minutes into account #6650

Closed
luixxiul opened this issue Jan 15, 2017 · 13 comments · Fixed by #7489
Closed

Fixed "Estimated Time Saved" to take minutes into account #6650

luixxiul opened this issue Jan 15, 2017 · 13 comments · Fixed by #7489

Comments

@luixxiul
Copy link
Contributor

luixxiul commented Jan 15, 2017

Test Plan

  1. Launch Brave and visit a few sites which have ads or trackers (http://cnn.com, https://slashdot.org/)
  2. Quit Brave. Make sure all instances of Brave are closed
  3. Open your session file (~/Libraries/Application Support/brave/session-state-1 on macOS, %appdata%\brave\session-state-1 on Windows) in an editor such as Sublime, vim, notepad, etc. If you have a JSON formatting plugin, you may wish to use it (so that it's easier to edit)
  4. Edit the JSON foradblock > count to be 555555 (six fives)
    screen shot 2017-03-10 at 4 58 46 pm
  5. Save the file and then launch Brave
  6. Verify it shows hours with 1 decimal point of precision, like so:
    screen shot 2017-03-10 at 4 58 12 pm
  7. Close Brave and re-open your session file in your editor of choice
  8. Edit the adblock > count value to be 55555555 (eight fives)
  9. Save the file and then launch Brave
  10. Verify it shows the days with two digits of precision, like so:
    image

Original issue description

Describe the issue you encountered:

The new tab page has been displaying Estimated Time Saved 2 hours for some time now.

https://community.brave.com/t/estimated-time-saved-stuck-at-2-hours/735

I noticed it as well.

Expected behavior:

  • Platform (Win7, 8, 10? macOS? Linux distro?): macOS

  • Brave Version (revision SHA): 0.12.15

  • Any related issues:

@darkdh
Copy link
Member

darkdh commented Jan 16, 2017

I feel the same way

@luixxiul luixxiul added this to the 0.13.1 milestone Jan 16, 2017
@luixxiul
Copy link
Contributor Author

I added the milestone to 0.13.1

@bbondy
Copy link
Member

bbondy commented Jan 28, 2017

How many ads and trackers do you have?

@bbondy
Copy link
Member

bbondy commented Jan 28, 2017

@bradleyrichter this seems like just a UX issue because the next hour limit isn't attained very easily but people still want to see progress. Removing milestone for now though since I don't believe it to be a bug, but I'd like it to be improved though.

@bbondy bbondy removed this from the 0.13.1 milestone Jan 28, 2017
@bradleyrichter
Copy link
Contributor

bradleyrichter commented Jan 28, 2017 via email

@bsclifton bsclifton changed the title "Estimated Time Saved" seems to be stuck at 2 hours "Estimated Time Saved" should take minutes into account Jan 29, 2017
@bsclifton
Copy link
Member

Updated the title 😄 It would be good if (whatever the unit is), the next smallest unit could be used as a decimal. Like 2.5 hours. Truncating it to "2 hours" is definitely confusing and has caught me making faces like 🤔

@bsclifton bsclifton added this to the contributor backlog milestone Jan 29, 2017
@bbondy
Copy link
Member

bbondy commented Jan 31, 2017

Eventually someone will hit days so I think @bsclifton's idea is best, thougths @bradleyrichter ? Just no decimal if it's minutes or smaller.

@bradleyrichter
Copy link
Contributor

i think the hour truncation was just a side effect of the overall truncation and yes, we can special-case hours to use a decimal place. This should solve the problem at hand.

@maazadeeb
Copy link
Contributor

Hi guys,

I would like to take this up. 😄 Correct me if I'm wrong about the expectation

if time < 1 hour
    stay with current behaviour
else if time < 1 day
    show 1 decimal place for hours
else
    show 1 decimal place for days

@bbondy
Copy link
Member

bbondy commented Feb 5, 2017

sgtm, or even 2 decimal places for days. @bradleyrichter thoughts?

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Feb 5, 2017 via email

@maazadeeb
Copy link
Contributor

Awesome! The final expectation, for future reference:

if time < 1 hour
    stay with current behaviour
else if time < 1 day
    show 1 decimal place for hours
else
    show 2 decimal places for days

maazadeeb added a commit to maazadeeb/browser-laptop that referenced this issue Mar 4, 2017
Fixes brave#6650

Time saved in hours will now have 1 decimal place. And time saved
in days will have 2 decimal places

Auditors: @bbondy @bsclifton
maazadeeb added a commit to maazadeeb/browser-laptop that referenced this issue Mar 4, 2017
Fixes brave#6650

Time saved in hours will now have 1 decimal place. And time saved
in days will have 2 decimal places

Auditors: @bbondy @bsclifton
@bsclifton bsclifton modified the milestones: 0.13.6, contributor backlog Mar 11, 2017
@luixxiul
Copy link
Contributor Author

I thought the fix would be 7 hours 48 minutes with the test case in the 1st post.

To be honest I cannot figure out how long 0.15 day is, if it is not converted to hours and minutes...

@alexwykoff alexwykoff changed the title "Estimated Time Saved" should take minutes into account Fixed "Estimated Time Saved" to take minutes into account Mar 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.