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

feat: Order steps are missing tax information. #7342

Merged
merged 15 commits into from Jun 28, 2021
Merged

feat: Order steps are missing tax information. #7342

merged 15 commits into from Jun 28, 2021

Conversation

iamdevvalecha
Copy link
Member

@iamdevvalecha iamdevvalecha commented May 26, 2021

Fixes #7308

Short description of what this resolves:

tax info in details is added to order summary page.

Changes proposed in this pull request:

simplescreenrecorder-2021-05-26_16.05.01.mp4

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)

@auto-label auto-label bot added the feature label May 26, 2021
@vercel
Copy link

vercel bot commented May 26, 2021

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/TFndc9RwGBJi2zdz7okRpc9MVkXq
✅ Preview: https://open-event-frontend-git-fork-iamdevvalecha-tax-info-eventyay.vercel.app

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #7342 (2b1b00a) into development (014f590) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7342      +/-   ##
===============================================
- Coverage        20.34%   20.29%   -0.05%     
===============================================
  Files              582      582              
  Lines             6670     6670              
  Branches           149      149              
===============================================
- Hits              1357     1354       -3     
- Misses            5286     5289       +3     
  Partials            27       27              
Impacted Files Coverage Δ
app/components/tabbed-navigation.js 33.33% <0.00%> (-20.00%) ⬇️

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 014f590...2b1b00a. Read the comment docs.

@MeghalBisht
Copy link
Member

Show the price as well and the same UI as ticket page

@iamdevvalecha
Copy link
Member Author

previous:-

Screenshot from 2021-05-28 14-28-53

after changes:-
Screenshot from 2021-05-28 15-22-34

@iamdevvalecha
Copy link
Member Author

@mariobehling @SundaramDubey @MeghalBisht please review!

@MeghalBisht
Copy link
Member

MeghalBisht commented Jun 11, 2021

When tax is included in price.
Current
Screenshot from 2021-06-11 21-51-53

Your changes
Screenshot from 2021-06-11 21-51-51

Expected - When tax is already included, no need to show any +00.0

@iamdevvalecha
Copy link
Member Author

When tax is not included in ticket price:-
Screenshot from 2021-06-12 01-28-07
When tax is included in ticket price:-
Screenshot from 2021-06-12 00-46-07

@iamdevvalecha
Copy link
Member Author

iamdevvalecha commented Jun 11, 2021

@MeghalBisht @maze-runnar @mariobehling please review now !

@MeghalBisht
Copy link
Member

@iamdevvalecha Despite repetitive reviews you still push code with wrong indentation. Now, with this comment this PR is getting reviewed 4th time for the indentation thing. This just increases the number of request review counts for a trivial issue and consumes a lot of time than it should deserve.

@MeghalBisht
Copy link
Member

MeghalBisht commented Jun 11, 2021

When tax is included I don't think we should show it separately else what will be the difference in the two cases. I am not sure though. Discussed.

@iamdevvalecha
Copy link
Member Author

now?

@MeghalBisht
Copy link
Member

Wrong

Copy link
Contributor

@sachinchauhan2889 sachinchauhan2889 left a comment

Choose a reason for hiding this comment

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

please increase width of tax column.
Don't show tax info (0.00) if tax in not enabled.

32
30

@MeghalBisht MeghalBisht merged commit afa00c5 into fossasia:development Jun 28, 2021
@iamdevvalecha iamdevvalecha deleted the Tax-info branch June 28, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Order Process: Order steps are missing tax information
4 participants