search

sugarlabs / convert

Sugar units conversion activity
GNU General Public License v3.0
0 stars 6 forks source link

Addition of currency converter #26

Closed kushagra98 closed 5 years ago

kushagra98 commented 5 years ago
  1. Added real time currency conversion.
  2. Exponents were too small to see, hence made it square or cubic.
  3. Real time currency is through API (fixer.io)
quozl commented 5 years ago

Were all files added before commit? There's nothing in 3c6603e changes for a currency API.

quozl commented 5 years ago

Oh, never mind, I found it! Sorry.

quozl commented 5 years ago

Thanks. very interesting. I've read the terms of service and have some questions;

kushagra98 commented 5 years ago

Yes currently I have included my own API (Free Plan), its limit is 1000. Sorry I'm a student I cannot pay for the premium account 😢 Here is the screenshot of details

Screenshot 2019-04-16 at 6 59 05 PM

chimosky commented 5 years ago

Although it would be nice to have a currency converter added to the activity but the down side is that the user would need an internet connection for it to work.

As you have noticed, all conversions happen within the activity and do not require internet connection. I understand that the usefulness of the api would be to always keep the conversion rates updated but like I said above, not every user has a regular internet connection. And the rates can't be hard coded as they change regularly and would always require a patch to update.

kushagra98 commented 5 years ago

I don't think internet will be an issue. There are other apps like "Weather" which requires internet as well. Internet, now a days, is a basic part of learning.

kushagra98 commented 5 years ago

While switching between the toolbar unit ranges and starting the app, nothing was selected as default unit. It is not fixed.

kushagra98 commented 5 years ago

Fixed the issue of trailing zeros after the decimal point. Example writing 4 gave 4.0 which is equal to 4.0000001 which is fixed.

Kindly check it and review it. Thanks :)

chimosky commented 5 years ago

I don't think internet will be an issue. There are other apps like "Weather" which requires internet as well. Internet, now a days, is a basic part of learning.

Assuming that internet connectivity isn't an issue doesn't mean it isn't, sugar is used in different countries and not all have regular internet access.

kushagra98 commented 5 years ago

All the 5 issues stated in the #24 have been solved. Kindly check it and review it.

kushagra98 commented 5 years ago

@chimosky I agree, adding the currency converter requires internet instantly and if it is not available, the app refuses to start. However, for countries where internet isn't an issue, this could be an add-on, because I searched in the activities there are apps which fetches data from Wikipedia and other sites. If it is too much of problem then we can drop the idea of it :cry:

chimosky commented 5 years ago

@chimosky I agree, adding the currency converter requires internet instantly and if it is not available, the app refuses to start. However, for countries where internet isn't an issue, this could be an add-on, because I searched in the activities there are apps which fetches data from Wikipedia and other sites. If it is too much of problem then we can drop the idea of it cry

I like the idea, it's a great addition. You could try checking if there's an internet connection before adding the currency converter.

One problem though is the subscription plan for the API, I can't speak for Sugar Labs if that'll be taken care of.

You can delete the commented out lines. You added 3 extra lines in 5a3eead, is there a reason for that? I'm not able to test your changes now, thanks for the fixes.

quozl commented 5 years ago

Thanks.

I'm responsible for keeping Sugar working for no more than three million laptops. Assuming one million are in active use, and assuming 10% of those download a new release of Convert, and 10% of those have internet access, and each user makes one currency query a day, the API demand could be up to 10,000 requests per day. As the same feature can be delivered using Browse and the Google Search query (just type "X to Y" where X and Y are standard currency abbreviations), it will be very difficult to get anyone to pay for the service. We also couldn't comply with the terms of service. I'm not willing to propose this to the board.

Please remove the currency changes, so we can merge the changes that aren't related to currency. Thanks for your work on this. It will help with https://github.com/sugarlabs/convert/issues/24.

kushagra98 commented 5 years ago

Thanks for the review @quozl Yeah the price is way too high, I agree. I have removed the currency converter. Now it can be merged to help with #24 Thanks

kushagra98 commented 5 years ago

Further I would like to propose, shall I add other unit conversions as well? Angles of circles : radians (rad), degrees (deg) Pressure : pascal (pa), torr (torr) etc. Energy : watt (w), joule (j), horsepower (hp) etc. Force : Newton (N), dyne (dyn) etc. Digital Storage : bit (bit), byte (byte), kilobyte (kB), megabyte (MB) etc.

chimosky commented 5 years ago

Feel free to add other units.

kushagra98 commented 5 years ago

The 'Convert' app is now complete. Kindly review it and merge the changes :smiley: Thank you

quozl commented 5 years ago

Thanks. Reviewed. Tested against https://github.com/sugarlabs/convert/issues/24

However also;

I was going to cherry-pick your patches, but I got lost. I'll defer that until you've had a chance to fix the above.

kushagra98 commented 5 years ago

This activity requires a lot of observation and precision of numbers, I must say. Now entering 9 shows 9 and not 9.0000001. (Sorry I missed that on the last commit) I have increased the precision of numbers as well to get more precise answers. And as you suggested,

  1. The default units are now SI units.
  2. Any unit to same unit conversion gives x ~ x. ("atm" on both sides shows the conversion of 9~9)
  3. The image of the floppy disk is now changed to the USB drive.

Ps. Believe me it is easier to debug the python code than to find a suitable SVG image for the toolbar :stuck_out_tongue:

quozl commented 5 years ago

Thanks for the changes. I'll be reviewing them later.

Ps. Believe me it is easier to debug the python code than to find a suitable SVG image for the toolbar :stuck_out_tongue:

Indeed. I feel that too. We have clear and regular need for graphic designers, with skills in making SVG icons for us, but none are involved. We've seen very few new icons in the past year.

On the other hand, we might have graphic designers who aren't watching this repository, so you might ask on the sugar-devel@ mailing list?

Also have a look at https://github.com/jrg96/sugariconify ... it exists for some reason, and is referenced by our Wiki.

kushagra98 commented 5 years ago

On the other hand, we might have graphic designers who aren't watching this repository, so you might ask on the sugar-devel@ mailing list?

Sure, once you review it and merge the changes, I'll ping there about it.

Also have a look at https://github.com/jrg96/sugariconify ... it exists for some reason, and is referenced by our Wiki.

I glanced through it. It converts normal SVG's to sugar icons. COOL

However I was going through this manual "http://write.flossmanuals.net/make-your-own-sugar-activities/package-the-activity/" and read about the sugar icons. While hunting for the icons for "Convert" over the internet, I got icons which were mostly black in color. What I did was, I added this line style="fill:#ffffff;stroke:#000000" inside every path tag of SVG file, to convert my downloaded icons to white(and resemble like Sugar Icons).

kushagra98 commented 5 years ago

Ping! @quozl

chimosky commented 5 years ago

Tested, works as expected. Thanks.