First Round of Updates from Oliver User Test

[garrettmflynn]

Beyond #547, this PR implements the following changes as noted in @oruebel's user test:

1. Allow "Enter" to interact with the form, where this will jump to the next input (except in the case of lists, which will be jumped...)
2. Note that keywords should be provided one at a time
3. Allow arbitrary groupings similar to the Conditional Requirements (e.g. age and DOB) but without the linked validation
   • Implement the "Institutional Info" group with Institution and Lab together
4. Note DOI format (among others) for Related Publications
5. Show the full path of the field below the original label on the Metadata forms, so it's easy to tell what fields are linked to which higher-level properties (e.g. Virus now has NWBFile next to it)
6. Update Data Formats page to align with suggestions

[CodyCBakerPhD]

Some conflicts here now

[CodyCBakerPhD]

Allow arbitrary groupings similar to the Conditional Requirements (e.g. age and DOB) but without the linked validation
Implement the "Institutional Info" group with Institution and Lab together

This looks great! A first step towards the 'auto-population from a DOI' functionality

Show the full path of the field below the original label on the Metadata forms, so it's easy to tell what fields are linked to which higher-level properties (e.g. Virus now has NWBFile next to it)

Not crazy about this - it's adding a ton of uninformative text to the metadata forms that creates visual clutter - what problem was this intended to solve?

Also, 'NWBFile' is an unusual reference outside of a programmatic context. It's the technical name for the neurodata type of the file itself, and the name of the Python parent class, but we usually refer to the file itself as 'NWB file(s)'

Also also these fields are actually written to the 'general' group of the file, not attributes of the file itself; likewise, subject info is technically 'general/subject/...', but I don't think that it really important to understand when inputting these values in a turbo-tax like way

[CodyCBakerPhD]

Allow "Enter" to interact with the form, where this will jump to the next input (except in the case of lists, which will be jumped...)

Feels great!

Note that keywords should be provided one at a time

LGTM

[garrettmflynn]

Not crazy about this - it's adding a ton of uninformative text to the metadata forms that creates visual clutter - what problem was this intended to solve?

When you open an accordion and fill things in, you might lose track of what general header (e.g. General Metadata, previously NWBFile, which is still the path identifier; Subject; etc.) a certain property is referring to.

I agree it comes with a bit of clutter and some reference inconsistencies. I could take it or leave it—though it was something that Oliver emphasized as a possible issue that I agree with. This is easily toggled on / off with a flag passed as JSONSchemaForm properties.

Alternatively, maybe we could take a different tactic? Like some sort of indicator that sticks on the top-right of the instance window that tells users what sub-property they are currently at?

[CodyCBakerPhD]

When you open an accordion and fill things in, you might lose track of what general header (e.g. General Metadata, previously NWBFile, which is still the path identifier; Subject; etc.) a certain property is referring to.

I agree it comes with a bit of clutter and some reference inconsistencies. I could take it or leave it—though it was something that Oliver emphasized as a possible issue that I agree with. This is easily toggled on / off with a flag passed as JSONSchemaForm properties.

Alternatively, maybe we could take a different tactic? Like some sort of indicator that sticks on the top-right of the instance window that tells users what sub-property they are currently at?

A toggleable setting would be fine with me, first major aspect design/layout aspect that leads down the road of fine detail configurability so that's fun

Indicator might also be a good idea

I do think that this could be separated out to a different issue with different proposed PRs to easily compare feel of different implementations; it's not as straightforward as the other simple improvements on this PR

[garrettmflynn]

Alright, I've toggled this behavior off for now. I'll open a new PR after implementing an indicator rather than the current method that replicates the displayed path alongside all JSONSchemaInput instances.

[CodyCBakerPhD]

'ev' = 'event'?

[garrettmflynn]

Unlike ogThis, both ev (or even just e) and el are somewhat standard abbreviations in Javascript, similar to Python's except Exception as e. See https://dev.to/rahulbhai9/every-abbreviation-used-in-javascript-explained-29p2

Though I understand that more explicit variables could be better. There're at least 10 instances of each of these in the code, so I'd change all of those if that's what you'd prefer.

Should I move forward with that? If so, I think it'll be more appropriate to seek out and remove all my abbreviations in a separate PR

[CodyCBakerPhD]

Though I understand that more explicit variables could be better. There're at least 10 instances of each of these in the code, so I'd change all of those if that's what you'd prefer.

Should I move forward with that? If so, I think it'll be more appropriate to seek out and remove all my abbreviations in a separate PR

Sure, if a single PR to adjust all abbreviations is easier for you

except Exception as e

We also try to avoid that; I always do except Exception as exception, the most we usually reduce to is exc. But in any case we're appealing to readability for those who might not be familiar with such patterns or if they once were perhaps they have forgotten; no reason not to just write out what you mean in any case

[garrettmflynn]

Got it. I'll get started with that abbreviation PR.

[CodyCBakerPhD]

DOI validation seems to have problems; I can't even submit the 'example' DOI successfully

Perhaps this is a cross platform regex issue? Note the Inspector only checks the start of the string (https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/src/nwbinspector/checks/nwbfile_metadata.py#L117), and now that I checked that I see it also does take a handful of specific URL domains too, forgot about those ("http://dx.doi.org/", "https://doi.org/")

[garrettmflynn]

I'd just mistakenly included the doi: part of the string in the example, I was automatically injecting this since it was the only one I thought we would be accepting.

I've updated the associated regex to allow any of those prefixes.

[CodyCBakerPhD]

OK LGTM - added one small extra bit to the description on the popup for the alternative https form and tested both with a real publication