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

Simplify the Specifier component #333

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Simplify the Specifier component #333

wants to merge 22 commits into from

Conversation

gaurav
Copy link
Member

@gaurav gaurav commented Aug 28, 2024

The Specifier component has some complex logic that allows users to modify the properties of a specifier in multiple ways, but this can lead to the specifier being overwritten just be opening it (#263). This PR sets about restructuring this component so that you can no longer modify the specifier just by opening it -- you have to explicitly modify something to trigger a modification action. (This is the minimum we need to close #263).

Now, we can't just remove all of this code, because the way in which Vue Store's data model works, we can't just plug the UI directly into the data object -- we have to call $store.commit('setSpecifierProps', {specifier, props}) or $store.commit('replaceTUnitForPhylogenyNode', {...}) to commit the results. So instead we need a model like this:

  1. When the component is loaded, we load all of its values into a bunch of local variables, which model fields in the UI (so changing the UI immediately changes these fields). This is handled in the method loadSpecifier().
  2. We watch all of these variables; if any of them change, we immediately update the underlying data object with the appropriate variables depending on the specifier type. This is handled in the method updateSpecifier().

Additionally, this PR:

  • Removes Apomorphy as an option for the specifier dropdown (closes Remove "Apomorphy" from specifier type as we've now implementing it elsewhere #331).
  • Changes the "Expand" button on the specifier to the clearer "Edit" (this still changes to "Collapse" when the editor is expanded).
  • Fields that could previously be edited has now been made readonly.
  • Renames the "Verbatim specifier" field to the clearer "Specifier label".

Closes #320: we don't include any special support for UUIDs, but we do now note that they can be used. They appear as collection IDs.

Example taxon:
Screenshot 2024-11-04 at 1 45 54 AM

Example specimen:
Screenshot 2024-11-04 at 1 46 29 AM

Example specimen with a UUID:
Screenshot 2024-11-04 at 1 27 01 AM

@gaurav gaurav changed the base branch from master to fix-references-to-defaultNomenclaturalCodeURI September 23, 2024 00:25
Base automatically changed from fix-references-to-defaultNomenclaturalCodeURI to master September 23, 2024 18:35
@gaurav gaurav marked this pull request as ready for review November 4, 2024 07:00
@gaurav gaurav requested a review from hlapp November 4, 2024 07:00
@gaurav
Copy link
Member Author

gaurav commented Nov 6, 2024

  • Might want to include taxon name within a specimen

    • However, they might conflict in the future: where a name and a specimen points to the wrong place.
    • Answer: make it clear that people can add a name to the label, but don't add support for a full taxon name just yet.
  • Should allow for annotations -- things curators might want to put in that don't have bearing on the reasoning.

    • We already have this at the phyloref level -- we should open a ticket about this but not implement it right away.
  • One outstanding bug: if you try to change the specifier label after changing phylorefs, it doesn't get reset, and you can end up in a situation where you can't delete the entire label.

@hlapp
Copy link
Member

hlapp commented Dec 22, 2024

@gaurav I'm not sure whether this is ready or not to be reviewed as is? You requested a review and it doesn't have draft status, but you're also mentioning an outstanding bug (implying that that needs to get fixed first?).

Perhaps the outstanding bug can be moved to its own issue and then reviewing this PR can go ahead with the caveat of the issue pending to be fixed? Or does this rather need to get fixed here?

@gaurav gaurav removed the request for review from hlapp December 23, 2024 03:52
@gaurav
Copy link
Member Author

gaurav commented Dec 23, 2024

Oops, my bad -- yes, I intend to fix the outstanding bug before sending it to you for review (I've removed the request to review now, and I'll re-invite you once it's ready for review). I think it's one of those bugs that indicate that there might be deeper issues here, so I'd like to try to fix it before finalizing this -- but if I see an opportunity to separate it into its own bug, I'll definitely do that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants