Add new config option for collections

[Saira-A]

Description of what you did:

Added a new option called "defaultToTreeIfCollection" to the config so that the left panel defaults to Index rather than Thumbnails if the item is a collection, so the tree view of manifests within the collection is visible by default

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
universalviewer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 9, 2024 10:41am

[demiankatz]

Thanks, @Saira-A! Once again, I'm not hugely familiar with the code here, but I gave this enough of a look to raise some questions. I hope the suggestions are helpful; please feel free to disregard anything that turns out to be a bad idea. :-)

[demiankatz]

Is this change necessary? This means that if defaultToTreeIfCollection is not set to true, the below behavior will be bypassed. Is that correct? I would think that regardless of default settings, collection-specific behavior would still apply... but maybe I don't fully understand the context.

[demiankatz]

Is it possible to leverage the defaultToThumbsView method to reduce redundant logic here, or is there a reason that won't work? (I confess that I considered the possibility without digging deep enough into the code to determine whether or not it is a good idea).

If that's not feasible, maybe you could at least use:

if (
    this.isCollection &&
    (this.config.options.defaultToTreeIfCollection || this.extension.helper.treeHasNavDates(treeData))
) {

So you don't need to have two separate if/else clauses for showing the tree view options.

[Saira-A]

Thanks @demiankatz! I refactored it but I'm not sure about line 286 so let me know if there's a better way :)

[demiankatz]

Thanks, @Saira-A! Regarding line 286, I think it would probably make sense to move the remaining logic inside defaultToThumbsView so the logic is consistent. Maybe you could change lines 622-624 to something like:

    // Default to tree view if the manifest is a collection and either the "default to tree if collection" setting is
    // on, or the tree has nav dates (which always require a tree):
    if (this.isCollection() && (defaultToTreeIfCollection || this.extension.helper.treeHasNavDates(treeData))) {
      return false;
    }

I'm not really familiar with the "nav dates" functionality, though, so I'm basing this suggestion on a logical refactoring of the code more than on direct experience with the intent behind the code... so please take my suggestion with the appropriate grain of salt.

[demiankatz]

Thanks, @Saira-A! This looks good to me now (though I haven't personally tested it); see below for some suggestions that might help clarify the relationships between the configuration settings. Feel free to accept, modify or ignore as you see fit. :-)

[Saira-A]

Makes sense, thanks @demiankatz!

[demiankatz]

Thanks, @Saira-A, I'm approving this so merging won't be blocked and because the logic makes sense to me -- but it would probably be a good idea to get some hands-on testing on this before merging it if anyone has time (assuming @LanieOkorodudu hasn't already done it). :-)

[LanieOkorodudu]

Thanks, @Saira-A, I'm approving this so merging won't be blocked and because the logic makes sense to me -- but it would probably be a good idea to get some hands-on testing on this before merging it if anyone has time (assuming @LanieOkorodudu hasn't already done it)

I just test it and it should be fine to merge @demiankatz

[demiankatz]

Thanks, @LanieOkorodudu!