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

dev/financial#105 Add CSS class onto the radio button payment processor options #15940

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

seamuslee001
Copy link
Contributor

…nt processor options to differenciate between the different options for themers

Overview

This adds in an additional class to the radio button options for choosing your payment processor with the payment processor type in the class. To allow themers to change the styling for each radio button depending on the payment processor type e.g. use the PayPal logo as the radio button perhaps for PayPal ones.

Before

No unique class between the options

After

Unique class

ping @mattwire @MikeyMJCO @agileware-fj

…nt processor options to differenciate between the different options for themers
@civibot civibot bot added the master label Nov 23, 2019
@civibot
Copy link

civibot bot commented Nov 23, 2019

(Standard links)

@homotechsual
Copy link
Contributor

homotechsual commented Nov 25, 2019

(CiviCRM Review Template DEL-1.2)

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
      • COMMENTS: Not a user facing change.
    • Documentation (r-doc)
      • ISSUE: The developer documentation should be updated.
      • COMMENTS: There may be developer documentation updates required.
    • Run it (r-run)
      • PASS: Clean rebuild of dmaster with PR applied -> payment processor classes work as expected.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@mlutfy
Copy link
Member

mlutfy commented Nov 27, 2019

"ISSUE: The developer documentation should be updated."

@MikeyMJCO What kind of documentation update do you recommend? In which section? (CSS classes have often had very little documentation.. which is not great of course, but at the same time, they are kind of self-explanatory?)

@homotechsual
Copy link
Contributor

It's more a case of "if there is documentation that refers to this - it should be updated" but there wasn't an option for that.

@jaapjansma
Copy link
Contributor

Betty and I are reviewing PR's and we came across this one.

We agree with @MikeyMJCO that documentation would be nice so that theme developers know how they could use this in their theme. But we don't know where. We have looked in the developer guide but there is no section for theme developers. The best place we found is https://docs.civicrm.org/dev/en/latest/framework/ui/ but not sure if that is the right section for this.

Therefor we think this PR could be merged and we think it would be nice to have some sort of guide on how to create a custom theme for contribution pages. But that is outside the scope of this PR.

@eileenmcnaughton or @mattwire are you able to merge this PR.

@mlutfy mlutfy changed the title dev/financial#105 Add in additional class onto the radio button payme… dev/financial#105 Add CSS class onto the radio button payment processor options Feb 19, 2020
@mlutfy
Copy link
Member

mlutfy commented Feb 19, 2020

(I'm OK with merging, but will let someone like Eileen or Matt weigh in, because it's not my area)

@mattwire
Copy link
Contributor

Docs would be nice, but it's pretty easy to find this out when theming a form.

@mattwire mattwire merged commit ce8dad0 into civicrm:master Feb 19, 2020
@homotechsual
Copy link
Contributor

The docs comment here is somewhat irrelevant - there are no docs currently talking about theming this area and really we wouldn't be documenting every classname anyway.

@seamuslee001 seamuslee001 deleted the dev_financial_105 branch February 19, 2020 20:24
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.

5 participants