- Issue created by @chrisolof
- Status changed to Closed: won't fix
over 1 year ago 6:25pm 6 October 2023 - π³π±Netherlands neograph734 Netherlands
Hi, thanks for the feedback. However, creating UI fields for all custom properties feels a bit overkill. There is also no support for the SOUND property, to name something.
The UI is designed to work for most use-cases. If you require other properties, I suppose you are better off overriding the module's template file (views-vcards-view-row-vcard.html.twig) in your own theme.
You can either misuse one of the other fields, or use a hook_preprocess_HOOK to add a new variable to the list. - πΊπΈUnited States chrisolof
Oh bummer. I was incorrectly assuming this module was open to complete v4.0 property support.
At any rate I've already created a patch to add NOTE property support, so I'll post it here for your consideration. Maybe it will help others with similar needs.
Thank you for the work-around advice.
- πΊπΈUnited States chrisolof
New patch attached, created against the latest 3.x branch code.
- Status changed to Active
over 1 year ago 9:20am 7 October 2023 - π³π±Netherlands neograph734 Netherlands
Oh bummer. I was incorrectly assuming this module was open to complete v4.0 property support.
Don't get me wrong, I am not against it. However, I don't think it would be compatible with the way this module is currently built. There would be endless lists of properties everywhere and people with overridden templates would not be able to see the newly added fields.
I suppose that the better way would be to overhaul the whole field assignment form. Instead of putting all the properties there, use a drop-down to the select the vcard property and then assign fields (to sub-properties if applicable). But then I suppose that all properties would have to be created as annontated plugins with their own form and rendered output. It would be quite the overhaul...
I'll try to have a look the coming days, but cannot promise that I will ever finish it...
- Status changed to Needs review
about 1 year ago 7:26pm 17 October 2023 - last update
about 1 year ago 2 pass - last update
about 1 year ago 2 pass - π³π±Netherlands neograph734 Netherlands
@chrisolof, I don't know why the commit messages don't show up here, but I have been playing in the issue fork. If you have some time, please give it a try. Adding the NOTE property is now a matter of copying one of the provided classes and making minimal changes.
Be aware that there is no upgrade path yet, so please try the module in a development environment.
- πΊπΈUnited States chrisolof
Oh this is very cool! I'll try and get you a review here next week. I really like the new plugin design.
- Status changed to Needs work
about 1 year ago 12:32am 4 November 2023 - πΊπΈUnited States chrisolof
This is really neat. The flexibility to add multiple types of each property (say a work and home phone number, for example) and configure them all independently per your unique requirements immediately struck me as a nice improvement over the previous, fairly hard-coded model.
I haven't done a full implementation test against this branch yet, but after some light testing and code review I spotted a few potential improvements we might want to make at the early stages here:
- Omit rendering of empty property lines. I believe the prior setup accomplished as much via conditional output in the template. I just pushed this change into the issue fork.
- I suspect the render() method in the Photo plugin is not necessary, given the render method in VcardPropertyPluginBase.
- I wonder if supportsHomeWorkType might be better held in the annotation config / plugin definition...
- It seems to me that the plugin interface could drop prepareOutputString(), as it seems to be more of just a helper-method in VcardPropertyPluginBase, with nothing outside of VcardPropertyPluginBase depending on it. The render method seems like the one to keep in the interface since it's what the rest of the system hooks up to for output from the plugin. It's possible a plugin could decide to render by other means (or more directly), without following the prepareOutputString() then render() model.
- Document return values on prepareOutputString() and render(). I took a stab at this in my push to the issue fork since the empty-checking was related.Again, I haven't done extensive testing, but the basics appear to be in place and working here. I would say this still needs work, especially given the lack of an upgrade path, and it probably deserves a major version change when it lands in a release, but it is definitely all coming together here.
- π³π±Netherlands neograph734 Netherlands
Thanks for taking the time to have a look!
- Omit rendering of empty property lines. I believe the prior setup accomplished as much via conditional output in the template. I just pushed this change into the issue fork.
This makes sense. Thanks.
- I suspect the render() method in the Photo plugin is not necessary, given the render method in VcardPropertyPluginBase.
The render method for the photo plugin is different because it wraps base64 encoded data at 64 characters instead of 75 which is the limit for other vCard properties per https://datatracker.ietf.org/doc/html/rfc6350#section-3.2.
- I wonder if supportsHomeWorkType might be better held in the annotation config / plugin definition...
I suppose I was a bit biased by how Views does this for display/row/style plugins. But it sure makes sense to move it.
- It seems to me that the plugin interface could drop prepareOutputString(), as it seems to be more of just a helper-method in VcardPropertyPluginBase, with nothing outside of VcardPropertyPluginBase depending on it. The render method seems like the one to keep in the interface since it's what the rest of the system hooks up to for output from the plugin. It's possible a plugin could decide to render by other means (or more directly), without following the prepareOutputString() then render() model.
I was definatly thinking of a new major relase, because the compatibility with the old template (and possible overrides people made there). The upgrade path is not impossible to write. It is just a long list of old properties to read and the new format to put them into. That only requires time...
- last update
9 months ago 2 fail - π·πΈSerbia vaish
Patch from #4 re-rolled for the 3.1.x version. Great work on the version 4 of the module!