Add Mixin option for adding defaults to options and columns objects, … #831
Conversation
6pac
commented
Aug 7, 2023
•
6pac
commented
- Add Mixin option for adding defaults to Grid Otions and Columns objects preserving external references to those objects
- closes Remove 'columns' and 'options' object replacement on initialisation #805
…preserving external references to those objects
|
Posting this provisionally. I can't get a compilation running here without spewing a thousand errors so can't test until I fix that. It's late - will try again tomorrow using Visual Studio (been using Notepad++ to this point). We'll see how the CI tests go. |
src/models/gridOption.interface.ts
Outdated
| @@ -209,6 +209,9 @@ export interface GridOption<C extends BaseColumn = BaseColumn> { | |||
| /** What is the minimum row buffer to use? */ | |||
| minRowBuffer: number; | |||
|
|
|||
| /** Use a mixin function when applying defaults to passed in option and columns objects, rather than creating a new object, so as not to break references */ | |||
| mixinDefaults: boolean; | |||
There was a problem hiding this comment.
can you make it optional ?:, I'm not entirely done with this interface. I had some issues with extending the interface in my libs, so I had to turn all options to optional in here so that I could extend properly in my libs. I'm still working on this though
| @@ -910,6 +918,7 @@ const SlickCore = { | |||
| hide: hide, | |||
| slideUp: slideUp, | |||
| slideDown: slideDown, | |||
| applyDefaults: applyDefaults, | |||
There was a problem hiding this comment.
I forgot to review these exports, in ES6 you can omit the right side if it's the same name as on the left. We should probably simplify them all
- applyDefaults: applyDefaults,
+ applyDefaults,
src/slick.grid.ts
Outdated
| @@ -3377,24 +3392,45 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e | |||
| * @param {Boolean} [suppressSetOverflow] - do we want to suppress the call to `setOverflow` | |||
| */ | |||
| setOptions(args: Partial<O>, suppressRender?: boolean, suppressColumnSet?: boolean, suppressSetOverflow?: boolean): void { | |||
| prepareForOptionsChange(); | |||
There was a problem hiding this comment.
Unless this function is local, which I don't think it is, I would assume that you need the this here because SlickGrid is now a class and in TypeScript you must always include the this (or super) in contrary to C# where it's optional
this.prepareForOptionsChange();
src/slick.grid.ts
Outdated
| activateChangedOptions(suppressRender?: boolean, suppressColumnSet?: boolean, suppressSetOverflow?: boolean): void { | ||
| prepareForOptionsChange(); | ||
| this.trigger(this.onActivateChangedOptions, { options: this._options }); | ||
| internal_setOptions(suppressRender, suppressColumnSet, suppressSetOverflow); |
There was a problem hiding this comment.
same comment, I think you need this.
src/slick.grid.ts
Outdated
| if (!this.getEditorLock().commitCurrentEdit()) { | ||
| return; | ||
| } | ||
|
|
||
| this.makeActiveCellNormal(); | ||
|
|
||
| //if (this._options.enableAddRow !== args.enableAddRow) { |
There was a problem hiding this comment.
do these comments need to be preserved?
|
OK, this should be a lot better. I got it running in VS and with |
|
I note however that despite having only edited two .ts files, I still get absolute stacks of modified .js and .map files from the auto generation: This is pretty annoying. Do you have any tricks for getting around this? Or do you use Source Control in VS and not see it? |
|
Yeah that's a bit the problem that we'll always get about the Also if people want to use CDN (like jsDelivr for example then In short, we still commit all the At the moment, I commit the Also note that I want to test this code in my lib first before approving it, I just want to make sure all my Cypress passes without any breaking change. I'll test it in a day or two |
|
No problems. I understand the need for this structure, was just querying how we deal with it. So in summary, I think in future I'll create a PR with my actual changes and a second PR to update all the autogenerated files for the first PR. I just think it's good to keep the 'files changed' tab in the original PR unobscured by the flow-on changes. Also just checking, does the CI build regenerate all the dist files before running the tests? Or is it just testing with One final note: I set option |
You can create a PR with the code change so that I can review but you can go ahead and push directly on the
Yeah I think I do run a full build in the CI to make sure that the PR is ok and from that build I assume it regenerates all the
I would probably prefer to have it non-breaking because I think that might have an impact with my use of locale for translations (I18N) since I think I update the locale text by using the columns pointers (by ref) and I think that if you enable this new feature that will probably break my translation part. I'll test that in a day or two like I said. All the Cypress tests are passing in your PR now but we don't have locale anywhere in this project, it's only in my own libs so there's more type of Cypress tests on my side. You can take a look at this Slickgrid-Universal Example 7 if you want to see how I use Locales, you'll see that after switching locale, it changes the column titles, the header menu and grid menu options and also context menu (basically any menu plugins) and is super important that this still works |
|
OK, great. I'd expect that the new feature will make references more persistant rather than less, but we'll see. Is the next major step after this to merge this project and Slickgrid-Universal? |
|
You should really take a look at the v5 Roadmap discussion to see what's left to do 😉 On the Slickgrid-Universal side, I got all my Cypress tests working couple days ago but still wish to review the TypeScript interfaces again, I need to extend a few of them from SlickGrid and I have some little issues with extending them (for example the BTW, do you like the next styling theme? and what's your impression about the new structure and everything in general (apart from the |
|
Sure, I have been keeping an eye on the roadmap, but I meant completely merging the two projects into a single one. That's not part of the process AFAIK, but it does seem like a logical long term step. Re the styling, it's definitely more modern, and the grid theme has been looking dated. If I don't seem overly excited, it's because most of my clients are businesses, and really styling is quite a low priority concern for them. Functionality is of prime importance. But I definitely will be using the new themes. Re the structure, you've done a great job of working out all the various use cases and being systematic with supporting them. Again, I'm stuck back here in the dark ages - I don't use npm, or any of the JS frameworks, tree shaking, modules or imports. So I'm grateful to have someone who is familiar with all that guide the way through. But I understand that I'm far from the typical developer, and that the support for all these other use case will be a great boost to the project. So, many thanks for that. There is only on more thing I think we really need to do now, and that is to provide better documentation. I've been really busy the last 18 months, and I'm consciously not taking on new work for a while so I can hopefully get some free time starting late this year. Documentation is something that I enjoy, although it's always amazing how time consuming it is. But I think that's the final barrier to Slickgrid being on par with things like Ag-Grid. |
|
Just Googled 'Browser-Sync', and it looks interesting, but will it run with ASP NET MVC projects? There is hot reload in VS, but it has all these bugs that make it very slow and it crashes every now and then, so I generally turn it off and manually refresh. The problem is that it has to recompile the c# in the .cshtml files so it's not as simple as processing some ts/js/html. |
|
Thinking we need some documentation around tools use. Because your tool chain is completely different to mine, I find that I never know if a given tool is relevant to or even compatible with my process. |
Hmm not really intending to merge them, what I've done so far with my libs is to try to move as much code as possible from SlickGrid (here) into Slickgrid-Universal (controls, plugins, editors, formatter, ...) but the main pieces that I have not moved or recreated into Slickgrid-Universal is these 4 files ( SlickGrid is like an unassembled IKEA product and Slickgrid-Universal is all assembled (includes a bunch of Editors, a bunch of Formatters and a bunch of Filters). The advantage of SlickGrid (unassembled) is that it can be very small if the user only needs a very basic grid with barely any features, on the other hand I built my libs to be a fully assembled product with all Editors, Filters, Formatters that I need but the cost to this approach is a bigger size and in my use cases I'm ok with this cost.
The main reason I added Browser-Sync was to reload the page every time a TypeScript file is recompiled, at the beginning I was annoyed by the constant refresh that I had to do after updating the code. Browser-Sync is mainly for development, you would rarely use it in Production.
Yeah I have a huge load of docs for my libs, for example see Slickgrid-Universal - Wikis, I spent a lot of time creating these docs to avoid having too many users opening issues that are more questions than issues, about how to do this or that or whatever... but the fact is that adding SlickGrid docs would basically bring nothing to my libs, so I've never focus on this myself and I always preferred to work on features and fixing bugs, like all these migrations and modernization, which does also help my libs as well. That could maybe be done in the future. As it stands now, I think that migrating to TypeScript and bringing ES6/ESM support was the last of the last thing left to do on this project. I think that after v5, we will have achieved everything that I could think of in terms of modernizing the lib, which is a good thing because after over 5 years of work on this lib, it's been a long while :) |
|
So I'm a little surprised but all my Cypress tests are still passing and the Locale switching still seem to work as intended. So I guess that I'm ok with making it the new default. So I'm approving the PR as it is 👍🏻 BTW, I renamed the title to include the |
|
OK, merging this now and will switch to For migration guide, note that
|
