search

eeditiones / tei-publisher-app

The main TEI Publisher app
https://teipublisher.com
GNU General Public License v3.0
68 stars 33 forks source link

Updates to reconciliation API authority connector #204

Closed awagner-mainz closed 2 months ago

awagner-mainz commented 1 year ago
joewiz commented 1 year ago

Sounds nice! Just some suggestions with an eye toward a cleaner PR, if it's not too much trouble:

  1. The version number commit should be removed (version numbers are typically changed when new releases are prepped, not in the course of a feature PR).
  2. Ideally, the merge commit should be removed. This could be fixed with by rebasing against master and performing a force push to your branch.
awagner-mainz commented 1 year ago

Sounds nice! Just some suggestions with an eye toward a cleaner PR, if it's not too much trouble:

  1. The version number commit should be removed (version numbers are typically changed when new releases are prepped, not in the course of a feature PR).
  2. Ideally, the merge commit should be removed. This could be fixed with by rebasing against master and performing a force push to your branch.

Cool, I will try to do as you say. I am not particularly experienced with rebasing, reverting and such, so I hope I'll not botch up everything. (The version number I had to increase in order not to get confused during my local testing, but it's obv no problem to set it back.)

Thanks for looking into it!

awagner-mainz commented 1 year ago

Is there any further action required on my part or are things taking their course? I don't mean to rush, just want to make sure things are not waiting for something to happen on my end.

wolfgangmm commented 1 year ago

Looks great. My only worry is that you reformatted annotations.js and annotate.html, so it's hard to see actual changes. We're also working on a new version of the annotation interface (see branch feature/annotation-ng) and I'd surely like to pick and merge your changes into the new version as well.

Did you change a lot in those two files?

awagner-mainz commented 1 year ago

:eek: 😱 Sorry, I did not mean to do the reformatting (probably it was an automatic pretty-printing at some point). I'll try and see if I can roll this back! Also, I think it will be good if I check the conflicts in the corresponding tei-publisher-components pull request (https://github.com/eeditiones/tei-publisher-components/pull/168), for the two pull requests kind of need to go hand in hand. And while I'm there, I might also have a look at the feature/annotation-ng branch. (Is there any information about the goals of this overhaul?)

So there is in fact things that have to happen on my end 😉 I'll report back!