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

SearchKit - Image field handler (Fixes dev/core#2781) #21225

Closed
wants to merge 3 commits into from

Conversation

kurund
Copy link
Contributor

@kurund kurund commented Aug 23, 2021

Overview

  • Add image field handler to support rendering of image fields for search kit

https://lab.civicrm.org/dev/core/-/issues/2781

@civibot
Copy link

civibot bot commented Aug 23, 2021

(Standard links)

@civibot civibot bot added the master label Aug 23, 2021
@kurund
Copy link
Contributor Author

kurund commented Aug 23, 2021

Here is a simple implementation of staff listing using grid layout.

screenshot-wp localhost_7979-2021 08 23-18_22_17

@colemanw
Copy link
Member

Rolando needs to work on his professionalism.

@colemanw
Copy link
Member

It seems to me that requiring a value for "path" makes the UX more complicated; in most cases I think it would be the same as the field value. In the case of custom field image uploads, I think it will be too complicated to calculate that value to expect the user to do it with tokens, so we ought to do it for them. In both of these cases I don't think we should expose the value to the user.

But one really good/important thing to allow admins to configure would be width and height. I'm thinking 2 number fields with placeholder text "Auto".

@colemanw colemanw changed the title image field handler implementation SearchKit - Image field handler (Fixes dev/core#2781) Aug 23, 2021
@kurund
Copy link
Contributor Author

kurund commented Aug 24, 2021

@colemanw

It seems to me that requiring a value for "path" makes the UX more complicated; in most cases I think it would be the same as the field value. In the case of custom field image uploads, I think it will be too complicated to calculate that value to expect the user to do it with tokens, so we ought to do it for them. In both of these cases I don't think we should expose the value to the user.

I do agree that it's complicated for users to construct urls especially for custom fields. I have not handled that scenario as it involves file checksum etc and I am afraid won't get chance to check on it right now. I have added placeholder in the code for the same.

In our use case we construct path for the image. Images are handled using separate extension hence it can't be auto calculated.

My suggestion would be to construct the path so users don't have to change it but keep it in the UI for the use cases where path can be altered such as ours.

But one really good/important thing to allow admins to configure would be width and height. I'm thinking 2 number fields with placeholder text "Auto".

This make sense. I have added this now.

@eileenmcnaughton
Copy link
Contributor

test this please

@colemanw
Copy link
Member

colemanw commented Aug 28, 2021

@kurund I've reviewed this. It works well except I found 3 problems:

  1. Browser was showing errors trying to fetch the wrong URL during Angular.compile, fixed with https://docs.angularjs.org/api/ng/directive/ngSrc
  2. Link checkbox was not working to display image as a link.
  3. I found the UX of having both a "Rewrite" option and a "Src" option confusing. In practice, this PR ignores the "Rewrite" field but does not hide it. I think it's better to use that field since it already exists, and remove "Src".

I've fixed all the above in #21300

@colemanw colemanw closed this Aug 28, 2021
@kurund
Copy link
Contributor Author

kurund commented Aug 31, 2021

@colemanw

thank you

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