Parsing Currency symbol in various places #2376
Conversation
| await render(hbs`{{currency-symbol data.event.paymentCurrency}}`); | ||
| assert.equal(this.element.textContent.trim(), '$'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@CosmicCoder96 @iamareebjamal , A little help needed, Travis says not able read the 'property symbol' , Is something wrong with the test i added or i am completely on a wrong path.
There was a problem hiding this comment.
@shreyanshdwivedi If you could see some possible error in this , Why travis is failing?
There was a problem hiding this comment.
@kushthedude Just a question, after removing this test, does the build passes?
There was a problem hiding this comment.
@uds5501 nope even after removing the test it tells unknown attribute ‘symbol’
|
@CosmicCoder96 There are many other places too where there is no currency parser or there is no data parser , Should i push commits of all such places in this PR or Should I open a new issue for every such place and then make a PR? |
|
@kushthedude Solve all the cases in this PR. Thanks! |
There are some places where the currency parser is not rendering and this is not letting other data values to be rendered , one of such place is given in #2402 , |
abhinavk96
left a comment
abhinavk96
left a comment
There was a problem hiding this comment.
@kushthedude Here's an idea. Do the parsing, where it doesn't break the code, and you are successfully able to modify the tests. Please do make sure that when you do the parsing on any given page, it does not break the page. i.e. just visit it yourself. The places where you are stuck, leave them. We don't have the resources and mentors at the moment to guide people through it. It is not productive to spend this much time on a trivial issue. It has been open for about a month now.
|
@CosmicCoder96 i have noted all the places where the currency-symbol breaks the page, and where it works fine and there are some places where it dont break the page but the symbol does not gets renders too. I will make changes to those places where it is gettin rendered and will take a indepth look into it later where some workaround needs to be done |
|
@kushthedude can you please open an issue which lists the cases which remain after this PR is merged. |
|
@CosmicCoder96 I will open the issue as soon as i get free from college😄 |
|
@CosmicCoder96 All the changes here are gettin rendered and not breakin the page |
|
but they still broke the build 🏗 alas ! |
|
@CosmicCoder96 Currency symbol is not defined for event-ticket-test.js why dont we fix the test in #2444 |
|
@kushthedude we don't merge PRs with failing builds. Remove the change which is failing the build. Add it to the list of places in #2444. That was the whole reason I asked you to open that issue. Just keep the changes which you can make without breaking anything including the build in this PR. I cannot spend more time on this. |
|
@CosmicCoder96 done |
3 participants
Checklist
developmentbranch.Short description of what this resolves:
-Currency symbol in add order and ticket summary was set to '$' by default.
-Changing the currency symbol as per the local currency.
Changes proposed in this pull request:
-Changed the standard '$' sign to local currency symbol.
Fixes #2225 , #2223