feat(web): Show lens model in the asset viewer detail panel #15460
Conversation
|
Thank you for the PR. I have two concerns about the changes that I don't know how to resolve.
I came up with an attempt for solutions, but I am still not very happy with it wrt the density of information. 
|
|
Would it be possible to crop the second line out and display the full length as ALT attribute? In the end the content of the second line from the iPhone is duplicated information anyway. Normally, the important lens information should usually be stated first by the manufacturer. |
|
I like the idea of clamping the line and providing the remaining text in the hover. The most important info does usually come first. I did initially think about doing this with a search icon, but was concerned that might be too visually noisy for a somewhat niche feature. I'm not a UI/UX designer, so I'm certainly open to any other ideas here, too. Showing the lens model is the most important part of this feature. If linking to the search makes things too clunky for the UI, then I can remove that. |
| title={$t('search_for_lens')} | ||
| class="hover:dark:text-immich-dark-primary hover:text-immich-primary" | ||
| > | ||
| {asset.exifInfo.lensModel || ''} |
There was a problem hiding this comment.
Don't think this logical OR is needed because of the if on 439.
| <a | ||
| href={getCameraSearchHref(asset)} | ||
| title={$t('search_for_camera')} | ||
| class="hover:dark:text-immich-dark-primary hover:text-immich-primary" | ||
| > | ||
| {asset.exifInfo.make || ''} | ||
| {asset.exifInfo.model || ''} | ||
| </a> |
There was a problem hiding this comment.
If neither the make nor model are set, will this still render the a? Seems like you'd want to do the check higher up, similar to line 439.
| const cameraSearchUrl = new URL(AppRoute.SEARCH, globalThis.location.href); | ||
| cameraSearchUrl.searchParams.set( | ||
| QueryParameter.QUERY, | ||
| `{"make":"${asset.exifInfo?.make}","model":"${asset.exifInfo?.model}"}`, |
There was a problem hiding this comment.
This'll not work if the make or model have double quotes in them. It's probably best to use JSON.stringify for the entire value. Same concerns with getLensModelSearchHref.
I also think this'll give you unexpected results if the make is unset or if the model is unset, since converting undefined to a string gives you the string literal "undefined".
Additionally, you could probably deduplicate the two get*Href functions further since everything but the value parameter of the set call is the same.
Does a helper function already exist to be able to specify the search URL? Quick digging reveals getMetadataSearchQuery, so I imagine you'd want to use that and/or whatever code currently calls this function.
Nice! Looking forward to have this PR merged! 🤞 |
Let's implement the elapsed text here and not add the search icon but keep the search feature. We can find a way to make an indicator nicer later |
|
Thanks Alex. I was away on travel yesterday, but I'll update the code with this — as well as with the changes that Snowknight26 suggested — this evening. |
|
I've updated the code with the lens model clamped to 1 line. I've also simplified the implementation a bit by using the existing getMetadataSearchQuery helper function. I also added additional error checking for situations where either camera make or model is blank. I tested this with some photos where I removed make or model with exiftool. Thanks @Snowknight26 for the review and your suggestions on how I could improve the code! |
…pp#15460) * Adds lens details to the asset viewer * Update lens detail search links --------- Co-authored-by: Alex Tran <[email protected]>
This adds the lens model information to the EXIF area of the asset view info detail panel. This is useful because the display of the focal length alone doesn't necessarily tell you what lens you used for a particular shot.
Additionally, I've made both the camera make/model and the lens model clickable links. These go to the search page with a query of either that camera make + camera model (if you click on the camera make/model), or the lens model (if you click on the lens model).
This is based on and inspired by the link part of #15049, which has been super useful to me.
This is my first PR and I'm a Svelte newbie, so please let me know if there's anything I did wrong. I ran the code through all the npm checks.