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

Make action button icons configurable #7166

Closed
wants to merge 7 commits into from
Closed

Make action button icons configurable #7166

wants to merge 7 commits into from

Conversation

vindert
Copy link
Contributor

@vindert vindert commented May 5, 2021

Subject

I am targeting this branch, because it is BC.

It make it possible to choose if icon will be shown on action buttons, in the list view.

This is my first pull request and first contribution to a project.
Any comments are welcome

Closes #6970

Changelog

### Added
Make action button icons configurable

@VincentLanglet
Copy link
Member

Nice PR !

Could you try something similar to https://github.com/sonata-project/SonataAdminBundle/pull/4968/files

When icon is selected it would add a <span class="sr-only"> instead of removing the text.

@vindert
Copy link
Contributor Author

vindert commented May 5, 2021

I've added the sr-only span tags.
And added it to the suggested doc pages. Included it on the docs/reference/action_list.rst file.

docs/reference/action_list.rst Outdated Show resolved Hide resolved
src/DependencyInjection/SonataAdminExtension.php Outdated Show resolved Hide resolved
src/SonataConfiguration.php Outdated Show resolved Hide resolved
VincentLanglet
VincentLanglet previously approved these changes May 5, 2021
@VincentLanglet
Copy link
Member

Please review @sonata-project/contributors


sonata_admin:
options:
# Choices are: text,icon or both (default)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Choices are: text,icon or both (default)
# Choices are: text, icon or both (default)

@@ -699,6 +699,20 @@ You need to add option ``show_mosaic_button`` in your admin services:
tags:
- { name: sonata.admin, manager_type: orm, group: admin, label: News, show_mosaic_button: false }

Icons on action buttons
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Icons on action buttons
Show Icons on Action Buttons

?

'both',
'icon',
'text',
])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
])
])

->enumNode('button_mode')
->defaultValue('both')
->values([
'both',
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use a more representative term here, while bringing more flexibility to this feature.
"both" is useful only in case where 2 options are allowed, but what if we decide to add more in the future?

Copy link
Member

Choose a reason for hiding this comment

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

What could we add in the future ? Prior to this change this was always button + text, I don't know what could be added.

Do you have another suggestion ?
An array with the value ['icon', 'text'] ? An option all ?

Copy link
Member

Choose a reason for hiding this comment

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

I think "all", "full" or something like that could be good choices.
BTW, maybe we could also use other name for the option, as the current one is not clear enough to me regarding its presentation purposes. Personally, the "mode" term makes me think about other options like "readonly=yes/no" or "action=edit/show".
I guess choices like "button_view", "button_view_mode", "button_content", "button_render" could work.

Copy link
Member

Choose a reason for hiding this comment

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

We could also add context to the list.
action_buttons or list_action_buttons seems better than button.

@VincentLanglet
Copy link
Member

Hi @vindert, do you have time to take in account the review ? :)

@VincentLanglet VincentLanglet dismissed their stale review July 11, 2021 17:49

name should be changed

@VincentLanglet
Copy link
Member

Closing in favor of #7325

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

Successfully merging this pull request may close these issues.

Option to get small link_action buttons.
6 participants