Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All items in grid appear highlighted after adding an item to the grid #1332

Closed
5 tasks done
rossmills99 opened this issue Dec 17, 2023 · 9 comments · Fixed by #1337
Closed
5 tasks done

All items in grid appear highlighted after adding an item to the grid #1332

rossmills99 opened this issue Dec 17, 2023 · 9 comments · Fixed by #1337

Comments

@rossmills99
Copy link

rossmills99 commented Dec 17, 2023

Describe the bug

After adding a new item to the grid, all other rows of the grid appear highlighted.

Reproduction

I've reproduced this in two ways:

  1. Using SlickCompositeEditorComponent as described in documentation
  2. Using gridService.addItem on AngularGridInstance.

Expectation

No rows should be highlighed after calling addItem.

Environment Info

Angular ^16.2.0
angular-slickgrid ^6.6.4
@slickgrid-universal/composite-editor-component ^3.7.1

Validations

@rossmills99
Copy link
Author

Further info, grid options are configured thus:

this.gridOptions = {
      enableAutoResize: true,
      enableSorting: true,
      datasetIdPropertyName: 'id',
      enableCellNavigation: true,
      editable: true,

      enableAddRow: true,
      enableCompositeEditor: true,
      enableRowSelection: true,
      autoResize: {
        container: this.gridContainer?.nativeElement,
        bottomPadding: 2,
        rightPadding: 2,
      },
      rowHeight: 48,
      externalResources: [this.compositeEditorInstance],
    };

I wasn't sure why but enableRowSelection: true, seems to be required for the composite editor to work. It seems like a strange requirement for adding a row though.

@rossmills99
Copy link
Author

Update:

  • the items are actually showing the highlight style, not selected (I just happen to use the same color for both).
  • another bug seems to be that the :hover style on rows is overridden by the highlight style, but for even rows only, so that hovering odd rows shows the :hover style when they are highlighted, but even rows it does not.

@rossmills99 rossmills99 changed the title All items in grid appear selected after adding an item to the grid All items in grid appear highlighted after adding an item to the grid Dec 17, 2023
@rossmills99
Copy link
Author

A workaround here is to set the GridServiceInsertOption.highlightRow to false in the call to addItem (it defaults to true).

Otherwise, it seems the bug is that all other rows are highlighted, instead of the newly added row.

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 18, 2023

I wasn't sure why but enableRowSelection: true, seems to be required for the composite editor to work. It seems like a strange requirement for adding a row though.

We use Composite Editor mostly, and strictly in our case, for Mass Update or Mass Selection updates hence why Row Selection is required by the plugin.

The row highlight after update is achieved by using SlickGrid item metadata change, it adds a CSS class named highlight for 1.5sec which simulates the highlight flashing. Perhaps the highlight is too generic of a name and is causing issues on your side because that CSS class is already defined for something else? I guess that it might be better to rename that class for something less generic, maybe slickgrid-highlight. Could that be the cause of your issue? For reference, the highlight is used in this line of the Slickgrid-Universal highlightRowByMetadata() method of the GridService... hmmm actually taking a look at the slick-bootstrap.scss file, the .hightlight on specific to .grid-canvas .slick-row .highlight (here), so even if it's a generic name it shouldn't cause issues with any other generic highlights.

For the other styling issues you mentioned, I think it would be easier if you contribute a fix, all the Composite Editor styling should be in Slickgrid-Universal in this file
https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/common/src/styles/slick-editors.scss#L154

Note however that I published v7.0 couple days and therefore I stopped supporting v6.x. It would be better if you migrate to v7.0, but note that it requires Angular 17

Without a full repro, it's hard to provide any help and the Example 11 is working as expected and has Cypress E2E tests so I cannot replicate without a full repro, perhaps you can test in Example 11 and provide code that fails with that demo

@rossmills99
Copy link
Author

I think I've discovered the source of my issue - the data are stored in the database with rowClass: 'highlight' because that value is being written to the dataContext. I am saving dataContext to the database in the onSave callback. Perhaps there's a better way to write the new row to the backend? I would not expect the dataContext to be mutated and have extra properties added to it. The code I have looks like:

this.compositeEditorInstance?.openDetails({
      headerTitle: modalTitle,
      insertOptions: {
       // highlightRow: false,
      },
      modalType,
      // insertNewId: 1234, // you can provide a custom Id (defaults to last Id+1)
      // insertOptions: { position: 'bottom' }, // if you wish to add the item to the bottom (defaults to top of the grid)
      onError: (error) => {
        console.error(error);
        alert(error.message);
      }, // you should define how to deal with error coming from the modal
      // you can optionally provide an async callback method when dealing with a backend server
      onSave: (formValues, selection, dataContext) => {
        console.log('saving', { formValues, selection, dataContext });
        return new Promise((resolve, reject) =>
          this.dataViews
            .upsertDataViewEntry(this.collection, dataContext.id, dataContext)
            .subscribe({
              next: (result) => {
                const updatedDocument = result.result;
                Object.assign(dataContext, updatedDocument);
                resolve(true);
              },
              error: (err) => reject(err),
            })
        );
      },
    });

I can see that rowClass propety is stored to the DB (via call to upsertDataViewEntry) although it is later deleted from the dataContext object so it doesn't actually show up in the console.log that I have there. It seems kind of fragile because if someone happens to use a field named rowClass then it could presumably break the grid display.

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 18, 2023

Yeah ok, it does use rowClass in the Grid Service method, the reason is because I followed this Stack Overflow answer on how to highlight a row. For the rare case that the data context is mutated (e.g. TreeData/RowDetail do mutate data context), I typically prefix these props with 2 underscores __rowClass, that should be better to avoid conflicts with user's code. I don't know if there's a better way to highlight, I just followed the SO like I said, so I can't say much about its fragility but I could use the __ prefix though

@rossmills99
Copy link
Author

Thanks for your quick response! For now, I think I can work around this knowing that the rowClass property can be ignored when persisting the newly added data.

I'm not too familiar with the underlying APis but maybe it would be possible to store metadata somewhere else, i.e. not on the data object itself. Perhaps a separate map, rowId -> metadata?

@ghiscoding
Copy link
Owner

So I looked at possibly reimplementing the row highlight and after investigating, I decided to address the issue directly in the SlickGrid core library (because with the new major release, SlickGrid is now merged in Slickgrid-Universal). I don't want to deviate the SlickGrid core lib too much but in this case I made an exception since in SlickGrid we already have a ref (row cache) to all the currently shown Slick rows in the grid (including their HTML Node ref), since we have that I can simply add/remove CSS classes for a certain duration period and voila! At the end of it, I no longer need the use of ItemMetadata (which indirectly required to mutate the item, as you pointed out as being fragile, and also no longer need to update the item in the grid), so performance wise it should also be a lot better. I also added a new grid option rowHighlightCssClass that is defaulted to highlight-animate and when in play, it will use CSS animation (similar to prior code but much lighter and it seems visually better too). You can see more info in the Slickgrid-Universal PR referenced above (including an animated GIF of the UI with the new code)

So all in all, this is much better but will only be available in Angular-Slickgrid v7.x (same note as I left in the other issue, I don't want to support older versions)

ghiscoding added a commit that referenced this issue Dec 21, 2023
- fixes #1332, #1333
- possibly (and hopefully) fixes #1334 by using the new reimplementation of node-extend that is used for merging grid options and other sections of Slickgrid-Universal object merging & extending
ghiscoding added a commit that referenced this issue Dec 21, 2023
feat: reimplement highlight row, node-extend also fixes #1332, #1333
@ghiscoding
Copy link
Owner

The row highlight should be a lot better with the new implementation in v7.1.0, you can see the updated demo Example 11 to test it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants