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

Add IDs to toolbar elements #16408

Closed
wants to merge 1 commit into from
Closed

Add IDs to toolbar elements #16408

wants to merge 1 commit into from

Conversation

Jako
Copy link
Collaborator

@Jako Jako commented Mar 11, 2023

What does it do?

Add IDs to toolbar elements in MODx.grid.SettingsGrid

Why is it needed?

If toolbar elements i.e. have to be hidden in a class that extends MODx.grid.SettingsGrid, Ext.getCmp needs an ID to access the toolbar element.

In #16089 the ID was removed. This PR fixes that, but with an ID that is variably set with the Ext.id of the current MODx.grid.SettingsGrid. This way multiple MODx.grid.SettingsGrid will not interfere with each other by using the same ID multiple times.

How to test

At the moment only in Agenda. There I extended the code to get the right toolbar elements.

before:

            afterrender: function () {
                var filterNamespace = Ext.getCmp('modx-filter-namespace');
                if (filterNamespace) {
                    filterNamespace.hide();
                }
            }

after:

            afterrender: function (cmp) {
                var filterNamespace = Ext.getCmp('modx-filter-namespace');
                filterNamespace = filterNamespace || Ext.getCmp(cmp.ident + '-filter-ns');
                if (filterNamespace) {
                    filterNamespace.hide();
                }
            }

Related issue(s)/PR(s)

#16089

If toolbar elements i.e. have to be hidden in a class that extends MODx.grid.SettingsGrid, Ext.getCmp needs an ID to access the toolbar element.

In modxcms#16089 the ID was removed. This PR fixes that but with an ID that is set variable with the Ext.id of the current MODx.grid.SettingsGrid. So multiple MODx.grid.SettingsGrid won't interfere with each other because of using the same ID multiple.
@Jako Jako requested review from opengeek and Mark-H as code owners March 11, 2023 17:03
@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Mar 11, 2023
@smg6511
Copy link
Collaborator

smg6511 commented Mar 11, 2023

Guys - FYI, a number of filtering deficiencies are handled in #16369, albeit in a different way. (As I re-read your description, I see the issue has to do with extending the grid so I'll need to take a look at that.) Please note that the best practice is to avoid specifying ids for components unless absolutely necessary. There are other ways to access specific components that have an itemId, which is the safer way to go.

@Jako - I have Agenda installed in my dev install and will test to see what's happening there. What's the bare minimum I'd need to set up in Agenda to see the issue in action?

@Jako
Copy link
Collaborator Author

Jako commented Mar 11, 2023

@smg6511 You can also use CrossContextsSettings 1.2.4 for the check.

The CMP of both extras has a dedicated system settings grid for the component. In this grid the namespace filter combo is hidden with the code examples above.

This ID drop should have been documented somewhere. And it should not be changed in a patch release.

If I read the documentation right, I just have to use the following code to avoid my patch:

            afterrender: function (cmp) {
                var filterNamespace = Ext.getCmp('modx-filter-namespace');
                filterNamespace = filterNamespace || cmp.topToolbar.getComponent('filter-ns');
                if (filterNamespace) {
                    filterNamespace.hide();
                }
            }

@Jako
Copy link
Collaborator Author

Jako commented Mar 11, 2023

Using cmp.topToolbar.getComponent('filter-ns'); works fine, so I will close this PR later on.

The '&tab' url property set by the new settings grid properties interferes with other extras, that don't have a tab handling or use the statCache.

@Jako Jako closed this Mar 11, 2023
@Jako Jako deleted the patch-1 branch March 11, 2023 20:49
@smg6511
Copy link
Collaborator

smg6511 commented Mar 11, 2023

The '&tab' url property set by the new settings grid properties interferes with other extras

Could you be more specific about this? Although the current change was not done by me, my open PR that I referenced also uses the tab key in the URL. I'd like to assess whether that key should be changed.

@Jako
Copy link
Collaborator Author

Jako commented Mar 11, 2023

Change the area in CrossContextsSettings 1.2.4 'system settings' tab (cog icon) to 'system'. Open the 'context settings' tab. Reload the manager. The 'system settings' tab is opened. This should be the case with every extra using an extended core grid that remembers open tabs etc. in the url.

@Jako
Copy link
Collaborator Author

Jako commented Mar 12, 2023

Is it possible to reactivate the id in 3.0.4, deprecate it and remove it in 3.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants