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

Support button elements in ajax popups #14136

Merged
merged 1 commit into from Jun 16, 2019

Conversation

@mattwire
Copy link
Contributor

commented Apr 26, 2019

Overview

This adds support for "button" elements in CiviCRM ajax popup forms in addition to the input type=button/submit and the "a class=button".

Related to https://lab.civicrm.org/dev/core/issues/347

This is a first step towards cleaning up button handling from a theming perspective as currently the ajax popups require buttons to be in the two specific formats and the "span" with an icon + link is notoriously difficult to theme.

This change opens up the possibility to replace "all" form buttons, for example by modifying "formButtons.tpl" like this: 9a05a2a - that part is trivial to do via an extension.

Before

Button types tightly coupled to ajax popups

After

Slightly more standard button types allowed for ajax popups. More flexibility for themes.

Technical Details

It is nearly impossible to sensible override crm.ajax.js via extension as it is tied to a specific order of loading here: https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Resources.php#L724 and things break if loaded later (eg. via extension hook).

Comments

@colemanw I would appreciate a review on this if you have time - hopefully you can see my intention. @vingle @christianwach @AkA84 feedback/endorsement welcome :-)

@civibot

This comment has been minimized.

Copy link

commented Apr 26, 2019

(Standard links)

@civibot civibot bot added the master label Apr 26, 2019

@christianwach

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@mattwire Makes sense to me 👍

@colemanw
Copy link
Member

left a comment

I think this is a good change but I had one question for @mattwire

js/crm.ajax.js Outdated Show resolved Hide resolved

@mattwire mattwire changed the title Support button elements in ajax popups WIP Support button elements in ajax popups Jun 13, 2019

@vingle

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I added the button class as well as this helps them render properly when using a bootstrap based theme.

tho @mattwire Bootstrap uses btn for class names? Foundation uses button.

Not wanting to piggyback on this to the wider question of front-end theme/css framework integration, but the first question on my interface notes here - https://pad.riseup.net/p/civicrm-interface seems a little relevant – should, & how should, there be css framework integration within Civi?

@mattwire mattwire force-pushed the mattwire:buttonpopups branch from a5c6ef0 to 72584dd Jun 16, 2019

@mattwire mattwire changed the title WIP Support button elements in ajax popups Support button elements in ajax popups Jun 16, 2019

@mattwire

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@colemanw A core change to crm.ajax.js in cdef34e#diff-13e5d322033454d3b6e51caa3d59ae86 means I either need to get this merged quickly or add some code to support old/new versions of CiviCRM in https://github.com/mattwire/civicrm-haystacktheme

Given this is now just a one-line change and has been reviewed already are you happy to merge @colemanw That way I don't need to override the file at all from 5.15/16 :-)

@colemanw

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Yes I'm happy with this change now.

@seamuslee001

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

test fail unrelated merging

@seamuslee001 seamuslee001 merged commit 23907a2 into civicrm:master Jun 16, 2019

1 check failed

default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.