fix(AnalyticalTable): Each column can have groupable, sortable, filte…

[gnseo]

fix(AnalyticalTable): Each column can have groupable, sortable, filterable options

[claassistantio]

All committers have signed the CLA.

[MarcusNotheis]

Hi @iamuan,
thanks for submitting this PR, great idea to add this feature on column level!
Could you please update the code to use the NullishCoalesingOperator instead of the 'manual' undefined check? This would increase the readability in my opinion.

Thanks!

[gnseo]

Hi, @MarcusNotheis.

As we know, the "undefined" usually means the property or argument is not provided by the caller.

So I thought it's better that if the property is not provided at column level, then the property at table level should be considered.

For increasing readability, how about add getProvidedArgument function like below.

function getProvidedArgument(...args){
  return args.reduce((result,arg)=>{
    if(arg !== undefined && result !== undefined){
      result = arg;
    }
    return result;
  },undefined)
}

groupable={getProvidedArgument(column.groupable, props.groupable)}
sortable={getProvidedArgument(column.sortable, props.sortable)}

[MarcusNotheis]

Hi @iamuan ,
the nullish-coalescing-operator is used for exaclty this type of issue:

The nullish coalescing operator (??) is a logical operator that returns its right-hand side operand when its left-hand side operand is null or undefined, and otherwise returns its left-hand side operand. (MDN).

So it is basically the same as the function you proposed, but JS-native and handling null as well. What would be the downside of using it? Do you have any objections?

[gnseo]

Hi, @MarcusNotheis .

I think the "undefined" and "null" have different meanings.
When we recognize if a property is provided from caller, usually check if it is "undefined".
And in my opinion, the priority of each property should be considered by this standard.

But also It's up to your choice, I would follow that.
Please leave a comment for this, I'll fix as your decision.

Thanks.

[MarcusNotheis]

Hi @iamuan ,

I see your point, but I would still go for the nullish-coalescing option. (We have the same pattern in some other places in the library, so if we would change it here we would have to adopt in all other places as well).
Thanks for your contribution!

[codecov-io]

Codecov Report

Merging #264 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #264   +/-   ##
======================================
  Coverage    73.9%   73.9%           
======================================
  Files         161     161           
  Lines        3533    3533           
  Branches      613     613           
======================================
  Hits         2611    2611           
  Misses        720     720           
  Partials      202     202

Impacted Files	Coverage Δ
packages/main/src/webComponents/Token/index.tsx	100% <ø> (ø)	⬆️
packages/base/src/Device/Media.ts	76.85% <ø> (ø)	⬆️
packages/main/src/webComponents/Title/index.tsx	100% <ø> (ø)	⬆️
...kages/main/src/webComponents/RadioButton/index.tsx	100% <ø> (ø)	⬆️
packages/main/src/webComponents/List/index.tsx	100% <ø> (ø)	⬆️
...ges/main/src/webComponents/BusyIndicator/index.tsx	100% <ø> (ø)	⬆️
...kages/main/src/webComponents/TableColumn/index.tsx	100% <ø> (ø)	⬆️
...ckages/main/src/webComponents/DatePicker/index.tsx	100% <ø> (ø)	⬆️
...nalyticalTable/virtualization/VirtualTableBody.tsx	84% <ø> (ø)	⬆️
...ckages/base/src/Scroller/ScrollContextProvider.tsx	100% <ø> (ø)	⬆️
... and 93 more

---

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 95f41e2...8bb13d1. Read the comment docs.

[MarcusNotheis]

LGTM. Thank you for your contribution!