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

Fix errors when hovering over the action column header #16484

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

theboxer
Copy link
Member

What does it do?

ExtJS is grabbing the header by regex on class 'x-grid3-td-([^\\s]+)', for some reason the class for action column header doesn't populate correctly (when using column model). Manually specifying an id fixes the issue.

Why is it needed?

Get rid of the errors

How to test

Open system settings & console. hover over the action column header.
CleanShot 2023-10-17 at 15 15 59

@theboxer theboxer added the bug The issue in the code or project, which should be addressed. label Oct 17, 2023
@theboxer theboxer added this to the v3.0.5 milestone Oct 17, 2023
@theboxer theboxer changed the title Fix errors when hovering action column header Fix errors when hovering over the action column header Oct 17, 2023
Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, this does indeed fix the error. I suggest an id that's more targetable (via css) and specific to the column's purpose. That it's an id is of no consequence in this case, because Ext just uses this internally and it doesn't end up creating that as an actual id in the DOM.

FWIW, the root cause of this is actually the different way the settings js class is constructed. I verified this via something else I'm working on where I've refactored that base class a bit where, among other things, the column model is just specified in the config (not explicitly created via new Ext.grid.ColumnModel as it currently is). In this scenario, the id is not needed on the actions column. That's why the error doesn't appear in any of the non-settings grids.

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

Probably worth double checking on pages with more than one grid.

@smg6511
Copy link
Collaborator

smg6511 commented Mar 26, 2024

Probably worth double checking on pages with more than one grid.

I double-checked this in the one area that has multiple grids that include an actions column (the ACLs) and all works as expected.

@smg6511 smg6511 added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Apr 3, 2024
@opengeek opengeek force-pushed the actions-column-fix branch from f19c50a to f9dd96d Compare April 5, 2024 15:27
@opengeek opengeek merged commit 4965924 into modxcms:3.x Apr 5, 2024
6 checks passed
@opengeek
Copy link
Member

opengeek commented Apr 5, 2024

I was unable to apply this to 3.0.x. If we want this fix on 3.0.x, I'll need the relevant changes submitted against that branch directly.

@opengeek opengeek modified the milestones: v3.0.5, v3.1.0 Apr 5, 2024
smg6511 added a commit to smg6511/revolution that referenced this pull request Apr 5, 2024
opengeek added a commit that referenced this pull request Apr 5, 2024
Backport of PR #16484 for 3.0.5.

---------

Co-authored-by: Jason Coward <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed. pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants