feat: Excel exporter will now observe if numeric type has dollar form…

[austinsimpson]

Hi @ghiscoding, I have another PR for you. This PR will change the excel exporter to observe the columnDef's formatter on numeric data types. If a column has a dollar formatter associated with it, the export will choose the dollarFormatter stylesheet. I was having issues where excel would interpret currency as a string rather than a number, breaking the sum, avg, etc functions.

[codecov-commenter]

Codecov Report

Merging #843 (076864a) into master (8a1bd35) will decrease coverage by 0.04%.
The diff coverage is 54.55%.

@@             Coverage Diff             @@
##            master     #843      +/-   ##
===========================================
- Coverage   100.00%   99.97%   -0.03%     
===========================================
  Files          239      239              
  Lines        16369    16377       +8     
  Branches      5776     5781       +5     
===========================================
+ Hits         16369    16372       +3     
- Misses           0        5       +5     

Impacted Files	Coverage Δ
packages/excel-export/src/excelExport.service.ts	98.42% <54.55%> (-1.58%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ghiscoding]

ok I see what you're trying to achieve, however I would prefer to do some refactoring and move all of that big switch case in a separate util file. Let me do that refactoring first and you can add the dollar formatters on top of my refactoring. The other reason I want to refactor is that you are missing unit tests and as it stands currently, it's not easy to add tests for them, so let me refactor first with easier way to add unit tests and you can add on top of it. I have to warn you though, you might have to redo your entire PR after my refactoring, I also do not want to lower my unit test coverage (I worked many months to get to 100%)

Here's a preview of my refactoring

A column with decimal formatter, which uses these option (see Common Formatter Options - Wiki) for currency with params like number prefix will now be interpreted my new refactoring and it stays as a valid number in Excel. I will do the decimal formatter only and you can add all the dollar formatters in a subsequent PR.

this.columnDefinitions = [
  {
    id: 'cost', name: 'Cost', field: 'cost',
    type: FieldType.number,
    exportWithFormatter: false,
    formatter: Formatters.decimal,
    groupTotalsFormatter: GroupTotalFormatters.sumTotalsDollar,
    params: { displayNegativeNumberWithParentheses: true, numberPrefix: '€ ', groupFormatterPrefix: '<b>Total</b>: ' /* , groupFormatterSuffix: ' USD' */ },
  },
];

[austinsimpson]

Not a problem, just let me know when your refactor is done. If I have to redo my PR, it's fine. It isn't a ton of code. Although, if I understand correctly, I may not need to add anything. I'll adjust your example above to work have a '$' instead.

[ghiscoding]

@austinsimpson
Actually let me merge your code and I'll extend it. After looking at the code and experimenting with what I showed in previous print screen, I think there's a lot of room for improvements

1. add custom format depending on the formatter (this is what you started with this PR but it can be extended with more formatters
2. currently the code checks and reapply format on every single cell and this is not too great for performance (which is which probably why it chokes on huge dataset). I'm investigating to see if it's possible to apply format on a column once instead of every single time (not sure if it's doable with excelbuildJS)
3. I could maybe also add a column definition option to provide excel custom format directly in columnDef

this will probably take a few days, but if I can get number 2., it would improve the performance and reduce download time

EDIT

Seems like number 2. is not possible, however the last PR I made improved Date format and isn't duplicating format anymore (it was prior to your PR and my latest PR). So anyway I can't seem to improve on that side but I'll be looking at number 3 and maybe add more format by field & format types.

[austinsimpson]

Hi @ghiscoding,

Thanks for getting this merged in. let me know if you'd like me to tackle #3. I could likely do it over the weekend at some point.

Also, when are you planning on releasing these changes? I'm trying to plan a release that depends on these changes.

Thank you,
Austin

[ghiscoding]

I'm still working on number 2, it's a bit problematic for grid with Grouping which is what I'm currently trying to figure out the best way. I'll do number 3 after that. I will take me a few more days, but that's ok I'm in vacations already so, taking my time

[ghiscoding]

@austinsimpson this is now shipped in the new Slickgrid-Universal v2.2.0 and Angular-Slickgrid v5.2.0 releases

Cheers and Happy Holidays ⭐

[austinsimpson]

Hi @ghiscoding, thank you for getting this out. Enjoy your holidays!

[ghiscoding]

@austinsimpson I was curious, have you had a chance to try the new Excel export options? I'd be interested to know if the new options are helpful. Cheers

[austinsimpson]

Hi @ghiscoding, happy new year! Yes, I was able to use it, and it was very helpful. Thank you!

[ghiscoding]

Great thanks for the confirmation 😉 ⭐