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

Remove Print Icon #15322

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Remove Print Icon #15322

merged 1 commit into from
Oct 9, 2019

Conversation

alifrumin
Copy link
Contributor

@alifrumin alifrumin commented Sep 18, 2019

Overview

Removes the print icon from the upper left hand corner of all pages because printing the page nearly always looks better then printing by clicking the print icon.

Before

Printer icon in the upper left corner of all pages
print Icon

After

No printer icon
no Print Icon

@civibot
Copy link

civibot bot commented Sep 18, 2019

(Standard links)

@civibot civibot bot added the master label Sep 18, 2019
@vingle
Copy link
Contributor

vingle commented Oct 7, 2019

Have opened a GL issue to discus this. Ping to @smaen123 - in case you want to add to it.

@vingle
Copy link
Contributor

vingle commented Oct 7, 2019

Hi @alifrumin - have chatted with @eileenmcnaughton and it sounds like there's three steps to get this merged. At the sprint there should be enough support to lift needs-concept approval, but before we see if someone can review the code, is there a reason it's still a WIP?

@alifrumin
Copy link
Contributor Author

Hey @vingle! I started this at a sprint and then completely forgot about it. I'm glad you guys have taken an interest in it. I will remove the WIP tag.

@alifrumin alifrumin changed the title WIP Remove print icon Remove Print Icon Oct 7, 2019
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Oct 8, 2019

As part of review - let's check if this makes https://lab.civicrm.org/dev/core/issues/1224 better or worse (since it DOES require css for printing - which might be removed here)

@vingle
Copy link
Contributor

vingle commented Oct 9, 2019

test this please

@vingle
Copy link
Contributor

vingle commented Oct 9, 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 majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We checked the icon was no longer visible on key screens. We checked you can still print rows from search. We checked print summary action still works from contact
  • Developer standards

    • Technical impact (r-tech)

      • UNREVIEWED
    • Code quality (r-code)

      • UNREVIEWED
    • Maintainability (r-maint)

      • UNREVIEWED
    • Test results (r-test)

      • PASS: The test results are all-clear.

@mattwire mattwire merged commit 491e51a into civicrm:master Oct 9, 2019
@mattwire
Copy link
Contributor

mattwire commented Oct 9, 2019

test fails unrelated

@eileenmcnaughton
Copy link
Contributor

Developer review portion of the review (@vingle )

  • Developer standards
    • Technical impact (r-tech)

      • PASS I've gone through the code and am comfortable that this only removes the print icon and the code that supports inserting & rendering it - and not any of the php that makes various print related functions work. I'm comfortable this won't impact other aspects.
    • Code quality (r-code)

      • PASS no new code introduced
    • Maintainability (r-maint)
      * PASS no new code introduced

@eileenmcnaughton
Copy link
Contributor

Thanks @alifrumin @vingle @mattwire - this appears to be something many people agree on based on sprint discussions

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

Successfully merging this pull request may close these issues.

4 participants