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

Added remembering filter properties of plugin events grid in browser url #15935

Closed
wants to merge 1 commit into from
Closed

Added remembering filter properties of plugin events grid in browser url #15935

wants to merge 1 commit into from

Conversation

Ruslan-Aleev
Copy link
Collaborator

What does it do?

  • Added the selected filter properties in the browser url.

plugin_events

Why is it needed?

Allow to copy/paste the state of the filter with the browser url.

Related issue(s)/PR(s)

#15186
#15185
#15184
#15183
#15182
#15181
#15115
#14086

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Dec 8, 2021
@Ruslan-Aleev Ruslan-Aleev added pr/review-needed Pull request requires review and testing. type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript. labels Dec 8, 2021
@Ruslan-Aleev Ruslan-Aleev added this to the v3.0.0-rc1 milestone Dec 8, 2021
@Ruslan-Aleev Ruslan-Aleev requested a review from Jako December 8, 2021 16:46
@JoshuaLuckers
Copy link
Contributor

In this case we need to remember the selected tab as well.

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Dec 21, 2021

I agree, I would add #tab-id to url for any LAST clicked tab (or collect an array of active tabs...), but this needs to be done in a separate js-component for MODX (which would work for any panels), and not just for the current panel. And if there is #tab-id when loading the URL, then the tab would become active.

But I don't know how to do it right. Maybe @smg6511 will help. It would be a very useful improvement, see #14086.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 21, 2021

OK, I'll put this on my plate to take a look at.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 29, 2021

@Ruslan-Aleev - OK, I have the solution for activating the correct tab. What's the best way to relay that addition to you for this PR?

@Jako
Copy link
Collaborator

Jako commented Dec 29, 2021

@smg6511 I won't add that with an anchor in the url, since there can be more than one tab group available i.e. two nested horizontal and vertical tabs groups.

@Ruslan-Aleev
Copy link
Collaborator Author

@Ruslan-Aleev - OK, I have the solution for activating the correct tab. What's the best way to relay that addition to you for this PR?

@smg6511 I mean GLOBAL improvement, not only within this PR. Somehow to keep track of active tabs, and take them into account when reloading the page.

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Dec 29, 2021

@smg6511 I won't add that with an anchor in the url, since there can be more than one tab group available i.e. two nested horizontal and vertical tabs groups.

I only meant the last active tab that the user clicked. Because this is most often in the grid, then at first glance this would be enough. Or in general, make this behavior only for grids.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 29, 2021

@Ruslan-Aleev @Jako - The functionality to remember the state of a page is already there (by setting and few params in a tab panel’s config; see the modx.panel.source.js file lines 25–32 for an example of how this is already implemented). If that's to be done, you'd probably want to give users the choice via a system setting; there are many page types (resource for example) where I'd find it somewhat annoying to land on a tab other than the primary one when opening any given document. To make this global, it would probably be best done within the MODx.Tabs class. I can play around with it.

But we’re talking about two different things: For the feature Ruslan’s introduced (of being able to pull up a grid panel’s filter “state” based on the URL’s query params), you need to do it in a way such as I've done for any page where the tab is not the primary one.

BTW, I'm assuming you saw the addition I made to the plugin panel class within the setup method (I wasn't sure of how to properly attack adding proposed changes to someone else’s PR ... guess I did it correctly?).

@smg6511
Copy link
Collaborator

smg6511 commented Dec 30, 2021

To make this global, it would probably be best done within the MODx.Tabs class.

After some initial tinkering with this, it's not as simple to implement as I had thought. One immediately noticeable issue within the elements panels having a code block (chunk, snippet, plugin, template) that is transformed by a code-editing plugin (Ace, CodeMirror, etc.) is that those plugins make certain un-ideal assumptions in their setup — leading to errors when loading the panel on a tab other than the one with that code block (which is always the first one). So, all extras that transform the code field would need to be tested and updated to accommodate this feature; they'd be very small changes, but this feature would be dependent upon those changes being made.

The other factor that comes into play is how the implementation should be designed; should each individual item be stateful or should the state be based on simply the panel’s class (e.g., if you were editing a template and the properties tab was open before navigating to another template, any subsequent templates would open to the properties tab until leaving a template page in a different state). How fine-grained should the statefulness be? Should the grids within maintain state.

Anyway, maybe some thought and consensus on exactly what the requirements of this feature should be would be prudent before diving into the coding of it.

There's one item (that I believe another user mentioned) that I would really like to be stateful on a very specific level: The per page limit on grids.

@Ruslan-Aleev
Copy link
Collaborator Author

e.g., if you were editing a template and the properties tab was open before navigating to another template, any subsequent templates would open to the properties tab until leaving a template page in a different state.

I think that each individual page (for example, a template page) has nothing to do with the tabs state of other pages, if I understand what it is about.
Now MODX, it seems to me, keeps the active tab for each page separately. For example, when editing TV, saving on the tab and then when opening TV again - the tab retains its activity (which was when saving).

@smg6511
Copy link
Collaborator

smg6511 commented Dec 31, 2021

Now MODX, it seems to me, keeps the active tab for each page separately.

Yes, as I've observed, only the TV panel maintains the page state of individual TVs. And that's the behavior I was asking about with my templates example. Would we want the state of every individual page (elements and resources) to be remembered, replicating the current behavior of the TV panel? Could it be problematic to do that for resources on sites with a large number of pages?

We may also want/need to look into persisting page state via LocalStorage instead of cookies (Ext 3 only has a cookie provider, not surprising as Ext 3 is over a decade old), especially if we're looking to be able to maintain the state of several items.

@Ruslan-Aleev
Copy link
Collaborator Author

Keeping tabs in this way is unnecessary, it is now confusing, as it seems to me (I would remove this behavior, leaving only for the tree) or maybe it is worth adding system settings, I'm not sure.
p.s. @smg6511 If you have additional information, then it is worth discussing it in the issue, because in PR it can get lost.

@Ruslan-Aleev
Copy link
Collaborator Author

@JoshuaLuckers @Jako As soon as you have time, please test PR, because even in the current version it is better than what we have now.
The tab can be clicked, and writing a tab processing script for each grid (there are several of them) is wrong, as it seems to me, and this is a task for a global solution.

@JoshuaLuckers
Copy link
Contributor

@Ruslan-Aleev sorry for the long wait, can you resolve the conflicts?

@Ruslan-Aleev
Copy link
Collaborator Author

@JoshuaLuckers Conflicts resolved, please check :)

@Ruslan-Aleev Ruslan-Aleev added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Feb 1, 2022
@Ruslan-Aleev Ruslan-Aleev modified the milestones: v3.1.0, v3.0.1 Mar 22, 2022
@opengeek opengeek modified the milestones: v3.0.1, v3.1.0 Apr 28, 2022
@opengeek opengeek closed this Jan 26, 2023
@Ruslan-Aleev Ruslan-Aleev reopened this Jan 26, 2023
@opengeek opengeek removed the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Jan 26, 2023
@Ruslan-Aleev
Copy link
Collaborator Author

There were conflicts in the files, I corrected them.

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. type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants