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 actions column to grids #14806

Merged
merged 2 commits into from Nov 29, 2019
Merged

Add actions column to grids #14806

merged 2 commits into from Nov 29, 2019

Conversation

theboxer
Copy link
Member

What does it do?

Added new properties to MODx.grid.Grid and MODx.grid.LocalGrid

  • showActionsColumn (default: true) - when set to true, shows the new actions column
  • actionsColumnWidth (default: 50) - width of this actions column
  • disableContextMenuAction (default: false) - when set to true the gear icon won't show up

Added new method that other grids extending Grid or LocalGrid can implement: getActions. It's getting these params: record, rowIndex, colIndex, store and should return an array or objects with properties:

  • action - name of the function to execute
  • icon - fa icon to use
  • text - tooltip

Those will get displayed as action buttons in the actions column.

I'm not 100% sure if this should be enabled or disabled by default, when it's enabled by default grids without any actions / items in getMenu will have to disable this. If it's enabled by default, grids with actions / items in getMenu will have to enable this.

Why is it needed?

Seems like context menus are not intuitive enough for people :)

Related issue(s)/PR(s)

Improves PR #14581 and #14125
Should resolve #14125

Screenshots

Contexts grid
Screenshot 2019-10-22 at 17 58 55

System Settings
Screenshot 2019-10-22 at 17 59 17

User Groups (Local Grid)
Screenshot 2019-10-22 at 17 59 32

@theboxer theboxer requested a review from Mark-H October 22, 2019 16:01
@JoshuaLuckers JoshuaLuckers added type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript. pr/review-needed Pull request requires review and testing. labels Oct 22, 2019
@Ruslan-Aleev
Copy link
Collaborator

Ruslan-Aleev commented Oct 22, 2019

Great PR, thanks! But it seems to me that in "Contexts" section it’s worth removing additional menu items, leaving only the gear (although I argued the opposite in other PR).
When I did my PR, I just took the "Contexts" grid for an example, and created the menu manually in the grid, and thought that in the other grids I would also have to add the menu in this way.
But since now the gear menu is automatically added, in "Contexts" section, additional menu items for consistency should be removed. And in the future, in separate PRs, we can add additional actions, if it necessary.

@Ruslan-Aleev
Copy link
Collaborator

Ruslan-Aleev commented Oct 22, 2019

In section (/manager/?a=source, /manager/?a=workspaces, /manager/?a=system/logs, /manager/?a=system/info (Database, Recent docs), /manager/?a=security/permission (Roles), /manager/?a=workspaces/lexicon), the gear menu doesn’t work (maybe in other sections).
It seems to me that it is worth adding an additional check.

@Ruslan-Aleev
Copy link
Collaborator

And it is worth renaming disableContextMenuAction to enableContextMenuAction, otherwise the logic is the opposite, which is strange.

@theboxer
Copy link
Member Author

I'm not sure what the options should be in the context grid. Seems like the edit action could be used quite often so might be worth it's direct icon in the actions column (not sure if it should get removed from the context menu or if it should be there as well - worth a discussion I guess).

Those sections you mentioned, they fall into the part where I'm not sure if this functionality should be enabled or disabled by default. If it will be enabled (the current state in the PR), I'll have to adjust all grids without a context menu and disable the actions column - as it wouldn't trigger any action (like you are reporting). Maybe I'm leaning towards disabling this feature by default (the opposite of the current state).

Yes it can be enableContextMenuAction, I guess it's just a matter of personal preference, I'll let others decide what they like, easy to change.

@theboxer theboxer added this to the v3.0.0-beta milestone Oct 23, 2019
@JoshuaLuckers
Copy link
Contributor

Would it be possible to have the gear show the same actions/options as the context menu?

This would also solve issues with devices that don't support a secondary click like mobile devices and tablets.

@theboxer
Copy link
Member Author

@JoshuaLuckers that's what it does. You don't have to define other actions to the new column and the gear icon will open the context menu (the standard one).

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

Nice!

I've made a small tweak to make sure it respects the width config property, and also disabled the gear icon on the packages grid. There'll probably be other places where it needs to be disabled, but I can see this giving grids a massive usability boost!

Mark-H added a commit that referenced this pull request Nov 29, 2019
…14806]

Merge remote-tracking branch 'upstream/pr/14806' into 3.x
@Mark-H Mark-H merged commit d708d0c into modxcms:3.x Nov 29, 2019
@Ruslan-Aleev
Copy link
Collaborator

@Mark-H For contexts, it’s worth removing unnecessary items, the gear icon is enough.
When I did my PR, I just took the "Contexts" grid for an example, and created the menu manually in the grid, and thought that in the other grids I would also have to add the menu in this way.
But since now the gear menu is automatically added, in "Contexts" section, additional menu items for consistency should be removed. And in the future, in separate PRs, we can add additional actions, if it necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants