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

fix: Hide currency when amount is 0 #5447

Merged
merged 11 commits into from
Nov 5, 2020
Merged

fix: Hide currency when amount is 0 #5447

merged 11 commits into from
Nov 5, 2020

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Nov 2, 2020

Fixes #5442
Fixes #5453

Short description of what this resolves:

$ sign was shown even when amount was 0 so fixed this issue.

Changes proposed in this pull request:

  • $ sign is not shown when amount is zero

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

BEFORE
Screenshot from 2020-11-02 11-03-23

AFTER
Screenshot from 2020-11-02 09-32-16
Screenshot from 2020-11-02 09-32-33
Screenshot from 2020-11-02 09-32-56

@vercel
Copy link

vercel bot commented Nov 2, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/cksudvdip
✅ Preview: https://open-event-frontend-git-ticket-box-5442.eventyay.vercel.app

@divyamtayal
Copy link
Member Author

@mariobehling @iamareebjamal please see the changes made and let me know to do any more changes.

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #5447 into development will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5447      +/-   ##
===============================================
- Coverage        23.23%   23.19%   -0.04%     
===============================================
  Files              493      493              
  Lines             5169     5182      +13     
  Branches            38       38              
===============================================
+ Hits              1201     1202       +1     
- Misses            3963     3975      +12     
  Partials             5        5              
Impacted Files Coverage Δ
app/components/public/add-to-calender.ts 4.54% <0.00%> (-4.55%) ⬇️
app/services/loader.js 39.56% <0.00%> (-0.44%) ⬇️
app/components/nav-bar.js 33.33% <0.00%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ea3a5f...9ca587e. Read the comment docs.

Soumi150
Soumi150 previously approved these changes Nov 2, 2020
@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 2, 2020

@mariobehling @iamareebjamal Please let me know if this is not the issue what I am thinking of.

@iamareebjamal
Copy link
Member

Please go ahead with this logic and solve other places in this PR as well

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 3, 2020

Fixed Sales(amount)
Screenshot from 2020-11-03 07-56-03

Fixed sales and bottom column sales
Screenshot from 2020-11-03 07-56-35

Fixed amount for orders and attendees
Screenshot from 2020-11-03 07-57-14
Screenshot from 2020-11-03 07-57-41(1)

Fixed Discount (currency not showing)
Screenshot from 2020-11-03 07-58-00

Fixed amount, subtotal (dashboard ticket page)
Screenshot from 2020-11-03 07-58-45

Fixed amount, subtotal (public ticket page)
Screenshot from 2020-11-03 08-00-03

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 3, 2020

@iamareebjamal You mean that like I have used if statement in different files, so there should be only one if or condition statement that should handle this globally?

@iamareebjamal
Copy link
Member

Yes

@divyamtayal
Copy link
Member Author

@iamareebjamal is this the correct way?
I modified currency-symbol file and passed one more arg to work accordingly
do let me know.

@divyamtayal
Copy link
Member Author

@iamareebjamal I have added the component pls see to it

@@ -0,0 +1 @@
{{#if (gt @amount 0)}} {{@currency}} {{/if}}{{@amount}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no space between currency and amount

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its there

@iamareebjamal
Copy link
Member

Half files are using component, half are not. currency-symbol helper is duplicated in all component calls

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

Half files are using component, half are not. currency-symbol helper is duplicated in all component calls

@iamareebjamal in some places I have to take care of only currency symbol where theres is no amount to be rendered, so for that I think I should not call the comp. and just do it there only?

@iamareebjamal
Copy link
Member

iamareebjamal commented Nov 5, 2020

Let's say you have a function which returns x^2 if x is odd and 1/x if x is even. Choose what the function and function call should look like:

  1. x if y else z | f(x^2, x % 2, 1/x)
  2. x if y % 2 else z | f(x^2, x, 1/x)
  3. x^2 if x % 2 else 1/x | f(x)

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

Let's say you have a function which returns x^2 if x is odd and 1/x if x is even. Choose what the function and function call should look like:

1. x if y else z | f(x^2, x % 2, 1/x)

2. x if y % 2 else z | f(x^2, x, 1/x)

3. x^2 if x % 2 else 1/x | f(x)

3
got it let me think of it

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

@iamareebjamal

@amount={{this.model.eventDetail.paymentCurrency}}
@amount={{sub ticket.price ticket.discount}}
@amount={{mult ticket.orderStatistics.tickets.completed ticket.price}}

so now there are diff cases where sometimes there is only arg directly passing or sometimes evaluating before and then passing as a single arg doing mul or sub
so should I handle this before calculation also in comp.??

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

@iamareebjamal I have some changes in the component

@iamareebjamal
Copy link
Member

so should I handle this before calculation also in comp.??

No


await render(hbs`{{currency-amount}}`);

assert.equal(this.element.textContent.trim(), '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for different cases

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok and rest changes in file is ok?

@iamareebjamal
Copy link
Member

Ensure there is a space between symbol and amount

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

Ensure there is a space between symbol and amount

See it's there and also same in various other places
Screenshot from 2020-11-05 14-42-52
Is this right?

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 5, 2020

Screenshot from 2020-11-05 15-12-03

Screenshot from 2020-11-05 15-26-05

@iamareebjamal I have added test, please let me know to make any changes.

@iamareebjamal iamareebjamal changed the title $ sign is not shown when Order amount is 0 fix: Hide currency when amount is 0 Nov 5, 2020
@auto-label auto-label bot added the fix label Nov 5, 2020
@iamareebjamal iamareebjamal merged commit cbcb620 into fossasia:development Nov 5, 2020
@divyamtayal divyamtayal deleted the ticket-box-5442 branch November 15, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants