Visual selector

[cokernel]

This is my attempt at satisfying #2 . I started with @GerZah's lity-based selector but added a controller to handle autocompletion and avoid the need to fetch all item records. There's still a fetchAll() on item types. I can remove or alter item type filtering if this is a problem.

[GerZah]

Sorry for the late reply, I just got back from my vacation. That's awesome, thanks – I'll look into it ASAP.
… I really don't think that the fetchAll() for the considerably smaller number of item types should pose a problem, we're talking about a handful to perhaps a dozen or two. The real threat was the fetchAll() on the actual items, which you addressed. Great!

... I still have a student assistant looking at the exact same thing, but having been away on vacation, I don't know yet where we stand with that.

[GerZah]

I'm sorry, I seem to be too stupid to figure this out by myself: Is there a way to download your whole modified plugin version as given in your 7th commit 6dff903? Thanks a lot in advance!

UPDATE: Got it –
https://github.com/omeka/plugin-ItemRelations/archive/6dff90338db719ade5f5d8ae9b8f548990366fa6.zip

[GerZah]

I love it. … I have a few i18n issues to remark, like "All", "Most recently updated", and "Alphabetically", and I certainly went back to my original extended (German language) i18n.

In addition to that, for my use scenario I had to merge this with my (closed) pull request #10 (re- Relation comments) – but except from a little tinkering, it all fell into place just fine.

My other modifications are of purely cosmetic nature:

• Firstly, relations are no longer hidden in the lower right of the page, but are shown together with the regular item properties.
• Secondly, relation titles are sorted differently, especially as I've upgraded ItemRelations to feature more than one custom relation vocabulary (don't ask – we need that, I can't help it).

All that went in with just a snap.

[cokernel]

I was avoiding the languages directory because I thought those changes had to be submitted through Transifex.

To keep the program i18n-friendly, should I just replace bare text with __('this function') ?

I wonder if adding a config setting for whether relations appear inline or in the sidebar would be too crufty. For the site I'm using that needs this plugin, there are actually so many item properties that using the sidebar actually highlights rather than hides the relations.

[GerZah]

Yeah, Transifex ... My Nemesis. ;-) I've been using a local version, I still didn't find the time to look into Transifex. But no problem.

But fact is that a lot of text portions in the original code are not i18n-ized. ... Yes, strings that are used inside the __("...") function will be translated automatically once the configured language contains a translation string in its .mo file.

A configuration setting is actually rather easy - I did exactly that in my #10 pull request. I could rather easily supply that if I find a couple of minutes.

... Do you care for my modified version of your rendition supporting relation comments? In that case I'd patch in the setting re- sidebar vs. content block and send you the whole thing back to you ... Not sure if I can commit something on top of your pull request, I had a hard enough time to get a full ZIP version of your pull request.

[GerZah]

Oh-kay. You could look at my commit GerZah/wesa_omeka@fd28d9c which now implements a configuration button to switch the relation display from the side bar to the main content. Mind you, I had already implemented that on top of your #11 pull request, so the latest commit modifies that back to be flexible based on a configuration switch.

But you'll get the gist (don't mind the relation_comment stuff if you don't care about that):

• ItemRelationsPlugin.php first creates an additional hook admin_items_show for displaying markup below the regular content instead admin_items_show_sidebar, which does it in the side bar.
  • A little further down it defines an additional configuration setting named item_relations_admin_sidebar_or_maincontent, stored to be in $adminSidebarOrMainconten – both in hookConfigForm() and hookConfig(). This is pretty much enough to make sure that the configuration dialog will be able to store it (see below).
  • Further down in here, the actual hook hookAdminItemsShow() is very similar to hookAdminItemsShowSidebar(), but they check the setting get_option('item_relations_admin_sidebar_or_maincontent').
• config_form.php implements the additional setting. ... Actually two, from your perspective, one again about relation comments, which you could ignore if you want to.
• Don't mind the de_DE.mo / de_DE.po for the time being, nor template.pot.
• item-relations-show.php is the actual view that will again ask the database for the current setting of item_relations_admin_sidebar_or_maincontent, and then display it, using a different CSS style and using h2 or h4, depending on where it is being displayed.

I am convinced that this should work nicely on top of your implementation as well.

[cokernel]

@GerZah, I'll take a look at these issues.

[zerocrates]

Is my understanding correct that this is effectively incorporated into #9?

[GerZah]

Hi, thanks for looking into this. Sorry, all that has been quite a while ago now, so I am having a hard time thinking back to when this pull request was crated.

ItemRelations is one of our core plugin, and it has undergone so many enhancements over the time. I totally agree that I should have submitted smaller pull requests, but not really hearing from you led me to the assumption that everything we did here was not really of interest. So I kept on adding enhancements, together with a couple of guys who actually pulled my fork – and, respectively, sent me pull requests to incorporate their latest changes.

All I can tell you is that my/our ItemRelations has since move so much further forward that it is probably hard to see much of the original code today. You might want to have a closer look at my latest commit 7a98b78, which is actually merely 3 days old.

I have no idea how this could, if desired, be married with the original ItemRelations code.

[zerocrates]

My specific question is more about the "visual selector" that was the initial title of this PR. Part of my hesitance with these was that we received two distinct versions of the same feature in different requests. In the original version of #9, it used a selector which fetched all the records at once, which was a problem keeping it from getting merged.

There's clearly been a significant amount of changes between now and then, though, and it seemed to me that at least to some extent the paginated selector from this branch had been included in #9. If that's the case, then the sensible course to me would seem to be closing this PR and focusing only on #9.

[GerZah]

Oh, absolutely! Some other contributor provided an AJAX view that paginated the display instead of the fetchAll(). This has been enhanced very much along the way, one of the latest improvement included a timer that prevents from firing the AJAX request too often, but waiting to update the display until a certain amount of (milliseconds of) time has passed without additional entries.

Yes. By all means, the original merge request should definitely me cancelled.

Gero Zahn, [email protected], http://www.gerozahn.de/

Am 21.07.2016 um 22:37 schrieb John Flatness [email protected]:

My specific question is more about the "visual selector" that was the initial title of this PR. Part of my hesitance with these was that we received two distinct versions of the same feature in different requests. In the original version of #9, it used a selector which fetched all the records at once, which was a problem keeping it from getting merged.

There's clearly been a significant amount of changes between now and then, though, and it seemed to me that at least to some extent the paginated selector from this branch had been included in #9. If that's the case, then the sensible course to me would seem to be closing this PR and focusing only on #9.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.