Skip to content

Allow corrections to fields and additions to empty fields#353

Open
tijmenbaarda wants to merge 23 commits into
developfrom
feature/corrections
Open

Allow corrections to fields and additions to empty fields#353
tijmenbaarda wants to merge 23 commits into
developfrom
feature/corrections

Conversation

@tijmenbaarda
Copy link
Copy Markdown
Contributor

This PR allows making corrections to existing fields and additions to empty fields, instead of comments on fields. The corrections and additions are also shown in the table and are used for filtering and sorting.

I also made some alterations to make it possible to save annotations by pressing return and dismiss the annotation editor as well as the modal with the escape key.

Copy link
Copy Markdown
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Good work! I'm mostly happy with this.

The one thing I find a bit counter-intuitive, when trying out the changes locally, is the way in which the pencil and addition buttons are placed. The addition button is only present on unset fields, while I would argue that it belongs on all fields, since a record might for example have multiple contributors. The pencil icon is only present on original text and not on corrections. The corrections can still be edited by clicking on them, but this is a difference with original text that I don't think users will naturally understand. They might get the impression that a correction is set in stone once made.

If this is tricky to address, you have my blessing to postpone it to a new issue so that Jeroen can get started annotating.

Edit: definitely postpone addressing the above point. See my recent comment in #293 on what I think should be done about this.

Other than that, I have some code comments. Do with them what you want. The only thing I insist you address is the hanging "if".

Comment thread frontend/vre/annotation/annotation.edit.view.js
Comment thread frontend/vre/annotation/annotation.edit.view.js
Comment thread frontend/vre/field/field.model.js Outdated
Comment thread frontend/vre/field/field.model.js Outdated
Comment thread frontend/vre/field/field.model.js Outdated
Comment thread frontend/vre/field/field.model.js Outdated
Comment on lines +76 to 81
} else if (_.isArray(value)) {
// Field is repeated: concatenate all values
return _.map(value, (value) => getMainDisplayOfFieldValue(value, this, annotations)).join(' ; ');
} else {
return getMainDisplayOfFieldValue(value, this, annotations);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't really need the elses here because every branch returns immediately. I won't insist that you need to remove them, though.

Comment thread frontend/vre/field/field.view.js Outdated
Comment thread frontend/vre/field/field.view.js Outdated
Comment thread frontend/vre/record/record.model.js
@jgonggrijp
Copy link
Copy Markdown
Contributor

@tijmenbaarda I have done the following so I could already deploy this feature to production:

  1. Create a release/0.4.0 branch based off develop (which already included the annotations field).
  2. Merge your feature/corrections into release/0.4.0.
  3. Deploy release/0.4.0 both to the acceptance server (as a sanity check) and to the production server.
  4. Create draft release notes for version 0.4.0 (https://github.com/CentreForDigitalHumanities/EDPOP/releases/tag/untagged-8997d211614d5e74bfe0), mostly just to document the fact that a 0.4.0 release is already in preparation.

Once you finalize this branch, you can normalize the situation again with the following steps:

  1. Merge this branch into develop as usual.
  2. Merge develop into release/0.4.0 (this might also bring in other changes that happened in the meanwhile).
  3. Locally check that everything seems to work.
  4. Deploy release/0.4.0 to the acceptance server again, as a final sanity check.
  5. Finish the 0.4.0 release as usual, creating a 0.4.0 tag on master.
  6. On the production server, before deploying, login as www-data, navigate to $WEBROOT/data/source and run git checkout master. (If you skip this step, the deployment script will attempt to merge the master branch into the local release/0.4.0 branch, which might fail.)
  7. Switch the target branch back to master in the dh_deploy_config.json and deploy as usual.

jgonggrijp added a commit that referenced this pull request May 18, 2026
jgonggrijp added a commit that referenced this pull request May 18, 2026
jgonggrijp added a commit that referenced this pull request May 18, 2026
jgonggrijp added a commit that referenced this pull request May 18, 2026
…es (#293 #353)

RecordField needs a reference to the whole Field instance
@jgonggrijp jgonggrijp mentioned this pull request May 18, 2026
tijmenbaarda and others added 6 commits May 21, 2026 11:44
Co-authored-by: Julian Gonggrijp <j.gonggrijp@uu.nl>
Co-authored-by: Julian Gonggrijp <j.gonggrijp@uu.nl>
Co-authored-by: Julian Gonggrijp <j.gonggrijp@uu.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants