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

SHOR-22: Remove extra PHP code for appending contact image - covered under shoreditch #2809

Conversation

swastikpareek
Copy link
Contributor

@swastikpareek swastikpareek commented Aug 1, 2018

Overview

HRUI component enforces a feature which actually fetch the image URL and appends it near the contact title. If we see Plain CiviCRM the Image is already appearing in the DOM we just need to move it to the position.

Since this change is required in CiviCRM too we have moved this feature to shoreditch - refer - civicrm/org.civicrm.shoreditch#264

Before

civihr - before

After

civihr - after

Copy link
Contributor

@AkA84 AkA84 left a comment

Choose a reason for hiding this comment

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

@swastikpareek this is approved, but you need to amend the PR description

you have a “Core overrides” section, but that section should not be there. You are not overriding core CiviCRM files, you are simply working on our own files.

Another observation about the "Technical Details" section: there isn't really much here to describe. This section is used to detail important aspects of your solution, but here you are

  1. Stating the obvious by simply copy-pasting the lines of code you removed
  2. Listing the code lines that you worked on. That's what the "Files changed" tab is for.
  3. Restating the reason for the changes. You already explained it in the "overview", there is no need to repeat it, especially in this section
  4. Mentioning the move of the feature to Shoreditch. Again you already say that and in any case it's not even a technical detail

In short: if you have something interesting/important to say to the fellow dev who will read this PR, then put it in the "Technical Details" section. If you don't have it, then do no add the section at all

@swastikpareek
Copy link
Contributor Author

Closing this PR as it is raised as with a different base fork. Refer - #2810

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

Successfully merging this pull request may close these issues.

2 participants