Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make all properties available in Tabulator records list, show record type with icon #233

Merged
merged 11 commits into from
Dec 4, 2024

Conversation

jgonggrijp
Copy link
Member

This implements half of #229 and half of #228. Showing record types or all properties in the detail view is not yet implemented, although some of the logic introduced in this branch could be reused for that purpose. I wanted to go all the way, but did not manage to go beyond the record list because of time constraints and distractions.

The autoColumnsDefinitions callback from frontend/vre/record/record.list.view.js was outfactored to frontend/vre/utils/tabulator-utils.js and then extended to always include all possible properties as available columns. The set of visible columns is still determined in the same way, though the decision to make a column visible now happens in a different place. For the extended logic, I used the MappedCollection feature from the freshly outfactored, still-to-be-released backbone-collection-transformers.

Then, I added an icon column to convey the record type: a person silhouette for a biographical record, a book for a bibliographical record. The HTML for the icon is rendered using a Mustache template. That template is potentially reusable for a Backbone.View. The HTML includes textual labels for screen readers. Record types are indicated at the individual record level rather than the catalog level, because I seemed to recall that some catalogs are mixed.

The new code lacks tests, but it has a low complexity and I hope it is sufficiently documented.

This branch has some side effects:

  • The docker-compose postgres service port is no longer forwarded to the host. This was never necessary to make docker-compose work. The reason to remove it is that it somehow prevented me from starting the container.
  • I turned the filter input for the Title column from a <textarea> back into a default <input>, because the height difference with other columns annoyed me.
  • I fixed a typo in the edpop-record-ontology.json that I discovered by coincidence.

As an aside, I notice that edpoprec:BiographicalRecord is a subclass of edpoprec:Record while edpoprec:BibliographicalRecord isn't. Conversely, BibliographicalRecord has a cardinality constraint while BiographicalRecord doesn't. I did not address this because I didn't know whether it was merely an oversight or whether there was a good reason for it.

Copy link
Contributor

@tijmenbaarda tijmenbaarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no comments on the code, but as always it was a pleasure to read and I learned a lot from it.

What I meant with #229 (and maybe I had not formulated it clearly) was also to show only the fields that belong to the specific record type (like extent, which would only show on BibliographicalRecord). However, I think that the code in this PR prepares well for this with the functions in tabulator-utils.js.

* {@link columnProperties}.
*/
const columnOrder = _.invert(_.keys(columnProperties));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I had no idea that this was possible!

@tijmenbaarda
Copy link
Contributor

As an aside, I notice that edpoprec:BiographicalRecord is a subclass of edpoprec:Record while edpoprec:BibliographicalRecord isn't. Conversely, BibliographicalRecord has a cardinality constraint while BiographicalRecord doesn't. I did not address this because I didn't know whether it was merely an oversight or whether there was a good reason for it.

This must have been an oversight of mine. I am busy with an updated record ontology and I will take this into account, too.

@jgonggrijp jgonggrijp merged commit 930466f into develop Dec 4, 2024
3 checks passed
@jgonggrijp jgonggrijp deleted the feature/always-all-fields branch December 4, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants