feat(plugin): new Row Based Editor

[zewa666]

this adds the new row based edit plugin

closes #1299

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a9cb8ee) 98.0% compared to head (f5f429c) 99.4%.
Report is 16 commits behind head on master.

Files	Patch %	Lines
...ackages/common/src/extensions/slickRowBasedEdit.ts	99.3%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1323     +/-   ##
========================================
+ Coverage    98.0%   99.4%   +1.5%     
========================================
  Files         198     199      +1     
  Lines       21286   21571    +285     
  Branches     7098    7203    +105     
========================================
+ Hits        20847   21429    +582     
+ Misses        439     142    -297     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zewa666]

ok I'm still not sure I really understand how to approach the i18n part.
Here's what I've tried so far:
i18n.patch

the mentioned function translateItems isn't really suitable (array based) plus it doesn't really do anything besides calling translate.

[ghiscoding]

ah sorry that wasn't a good explanation, without looking at the patch you linked (I'm supposed to be off on Sundays ;)). So anyway, I'll assume you might be looking for code when the locale changes, below is the code that calls that.

slickgrid-universal/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts

Lines 733 to 752 in 1d759f7

	// translate them all on first load, then on each language change
	if (gridOptions.enableTranslate) {
	this.extensionService.translateAllExtensions();
	this.translateColumnHeaderTitleKeys();
	this.translateColumnGroupKeys();
	}
	// on locale change, we have to manually translate the Headers, GridMenu
	this.subscriptions.push(
	this._eventPubSubService.subscribe('onLanguageChange', () => {
	if (gridOptions.enableTranslate) {
	this.extensionService.translateAllExtensions();
	this.translateColumnHeaderTitleKeys();
	this.translateColumnGroupKeys();
	if (gridOptions.createPreHeaderPanel && !gridOptions.enableDraggableGrouping) {
	this.groupingService.translateGroupingAndColSpan();
	}
	}
	})
	);

So you probably need to add a method that will be called to from the callback above.

If that is still not helping, just let me know and I'll take a proper look at the patch

[zewa666]

ok think I got it working. There was still a nasty bug with Cypress but I got a workaround to force drop the old cached items.
Next up ... writing docs

[ghiscoding]

I think you missed that one when I mentioned it couple days ago, I mean just remove & since it's not needed here

[ghiscoding]

same as previous, & can be removed

[ghiscoding]

I see a lot of the same code repeated, perhaps we could migrate this to an external protected function? Something along these lines

protected getTitleOrDefault(key: string, defaultTitle: string) {
  const actionBtnOptions = this.gridOptions.rowBasedEditOptions?.actionButtons; 
  const hasTranslateService = this.extensionUtility.translaterService?.translate;

  // another approach might be to move the line above to GETTERs in the class?
  
  return hasTranslateService ? actionBtnOptions[key] : defaultTitle;
}

this.btnUpdateTitle = this.getTitleOrDefault('updateButtonTitleKey', 'Update the Row');

I should probably do that in some other extensions like the GridMenu that I think I did something like you did and I think that this could make the code more readable.

[ghiscoding]

this is out of context but, it's a nice story... so I keep adding unit tests for the core files and it's not exactly a fun exercise but I keep finding and fixing small bugs and also removed a bunch of unused/unreachable code. So it does seem to be worth the extra work (I'm now at 99.4% coverage, the end of the tunnel is in sight woohoo)

[zewa666]

yeah its great when you see that type of payoff. I wasnt able to invest time the last few days sadly but hope to manage it in the next few ones.

[zewa666]

hey with the latest commit I've added again a bit of TS type inference helpers.

Way back than I've added already some to create the Column defintion type. So my question is whether it's ok to leave them all sprinkled around or you'd want to co-locate them all in one ts-utils.ts file sort of

[ghiscoding]

hey with the latest commit I've added again a bit of TS type inference helpers.

Way back than I've added already some to create the Column defintion type. So my question is whether it's ok to leave them all sprinkled around or you'd want to co-locate them all in one ts-utils.ts file sort of

ah yeah I saw the inference type you added, at first glance I thought it was a little overkill since it's only used once hahah but nonetheless it's quite powerful and you can't go wrong with that approach. Feel free to put it where you think is best, when I started the project, I used to create a file per interface but I see my folder is getting quite large now and lately, I started grouping some of them by feature depending on what it is. So you could add it to interfaces/rowBasedEditOption.interface.ts if it's only used for that or create something like inference.helper or whatever and if that is what you go with then maybe move the ones you've done in the past from column.interface. I don't have any preferences on which approach to choose, just go with what you think is best

slickgrid-universal/packages/common/src/interfaces/column.interface.ts

Lines 18 to 31 in 03cad4a

	type PathsToStringProps<T> = T extends string | number | boolean | Date ? [] : {
	[K in Extract<keyof T, string>]: [K, ...PathsToStringProps<T[K]>]
	}[Extract<keyof T, string>];
	/* eslint-disable @typescript-eslint/indent */
	// disable eslint indent rule until this issue is fixed: https://github.com/typescript-eslint/typescript-eslint/issues/1824
	type Join<T extends any[], D extends string> =
	T extends [] ? never :
	T extends [infer F] ? F :
	T extends [infer F, ...infer R] ?
	F extends string ? string extends F ? string : `${F}${D}${Join<R, D>}` : never : string;
	/* eslint-enable @typescript-eslint/indent */
	export interface Column<T = any> {

[zewa666]

yeah I'm a bit nervous when magic strings are floating around as my future self isn't that mindful of knowing what the intention was. So it serves as a real well gatekeeper to focus the type and be more expressive (all of that just to save one line of comment 🤣 )

Nevertheless the docs are now also added. A quick review on those would be very appreciated.

[ghiscoding]

"the Row based edting pluging", the word "edting" has a typo

[zewa666]

alright I think I got everything in that I've planned for. How do you want to proceed from here?

[ghiscoding]

looking good.... let's hit the Merge button 🚀

I think the next steps will be to replicate the new Example into the external repos (Angular-Slickgrid, ...), did you wanted to me to do that or did you wanted to do it too (for Angular I mean)?

[zewa666]

I can do so for Angular whenever you release slickgrid universal and bump it as dependency in Angular-slickgrid.