Scheme Label Editor & Reference Datatype Widget #169

johnatawnclementawn · 2024-12-31T01:57:07Z

Resolves #147

Requires Arches References #58

chrabyrd

Looks good! Just some pretty minor stuff -- happy to chat about about any of it

arches_lingo/src/arches_lingo/components/generic/LabelEditor.vue

arches_lingo/src/arches_lingo/components/generic/ReferenceDatatype.vue

arches_lingo/src/arches_lingo/components/generic/reference-datatype/ReferenceDatatypeEditor.vue

arches_lingo/src/arches_lingo/constants.ts

chrabyrd

lgtm!

chrabyrd

whoops, was only looking at a subset of changes. One moment 😅

chrabyrd

code looks good but there's some functionality issues:

the dropdowns do not work for a new scheme label

jacobtylerwalls

Looking great!

arches_lingo/urls.py

arches_lingo/templates/arches_urls.htm

arches_lingo/src/arches_lingo/types.ts

arches_lingo/src/arches_lingo/components/generic/reference-datatype/ReferenceDatatypeEditor.vue

arches_lingo/src/arches_lingo/components/generic/LabelEditor.vue

robgaston · 2025-01-08T20:15:35Z

I'm noticing that the tables for Labels and Statements appear to have regressed on this branch, on main I see type and language:

but on this branch they do not appear in the table:

…e config #146

This reverts commit d325db5.

jacobtylerwalls · 2025-01-15T18:18:12Z

arches_lingo/serializers.py

+            resource_instance_id = self.instance.resourceinstance.pk
+            resource_instance = self.instance.resourceinstance.__class__.as_model(
+                graph_slug, resource_ids=[resource_instance_id]
+            ).get(pk=resource_instance_id)


I'm looking into adding a developer convenience for this, either a method .enrich() and/or overwriting the .resourceinstance descriptor on the TileQuerySet so that it's a lazy getter that calls enrich() for you. 👀

(Not suggesting to block this PR on it.)

@jacobtylerwalls I swear this was working yesterday, but when I try to create a new label for an existing scheme self.instance is None. Would you be able to take a look at that in my absence?

Done! I think you and I were only testing updates, not creates. I also added a little developer convenience for now.

chrabyrd

Overall code looks good, some minor non-blocking nits there.

I'm noticing that I still can't add new labels, and the dropdowns powering the editor still do not show their selection if creating a new label.

Also I'm noticing this warning in the console now:

runtime-core.esm-bundler.js:202 [Vue warn]: Extraneous non-props attributes (pt) were passed to component but could not be automatically inherited because component renders fragment or text root nodes. 
  at <ReferenceDatatypeEditor value=null options= [{…}] multi-value=false  ... > 
  at <ReferenceDatatype value=null mode="edit" multi-value=false  ... > 
  at <LabelEditor value= {tileid: '7b559cf4-77a1-49b3-9116-aabffd464bd3', resourceinstance: 'b73e741b-46da-496c-8960-55cc1007bec4', nodegroup_id: 'ef87ac28-11de-11ef-9493-0a58a9feac02', sortorder: 0, provisionaledits: null, …} onUpdate=fn<update> > 
  at <SchemeLabel mode="edit" tileId="7b559cf4-77a1-49b3-9116-aabffd464bd3" onUpdated=fn<onSectionUpdate>  ... > 
  at <SchemeEditor key=0 editor-max=true editor-form="label"  ... > 
  at <SplitterPanel key=1 size=33 min-size=33  ... > 
  at <Splitter style= {height: '100%'} > 
  at <SchemePage onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< Proxy(Object) {__v_skip: true} > > 
  at <RouterView> 
  at <SplitterPanel min-size=25 style= {overflow-y: 'auto'} tabindex="-1" > 
  at <Splitter style= {height: '100%', overflow: 'hidden'} pt= {gutter: {…}} > 
   at <HierarchySplitter key=0 > 
  at <App>

but if these are planned to be addressed in followup tickets, I won't block.

chrabyrd · 2025-01-15T23:50:21Z

...o/components/generic/resource-instance-relationships/ResourceInstanceRelationshipsEditor.vue

@@ -10,6 +10,7 @@ const props = withDefaults(
    defineProps<{
        val?: string[];
        options?: ResourceInstanceReference[];
+        ptAriaLabeledBy?: string;


seems this shouldn't be an optional prop?

I think it should be optional because there are other ways of ensuring your forms are accessible and I don't think we should force use of aria labeled by

chrabyrd · 2025-01-15T23:51:04Z

..._lingo/src/arches_lingo/components/generic/non-localized-string/NonLocalizedStringEditor.vue

-const props = defineProps<{ value: string }>();
+const props = defineProps<{
+    value: string;
+    ptId?: string;


naming nit? what is a ptId?

Primevue InputText doesn't take an ariaLabeledBy prop like some of their other components, so instead I gave that component an Id, hence pass-through Id (ptId). Fixed in latest, but if we start to use these pass-throughs a lot more, I think pt as an object (pattern that primevue uses) is better than this.

jacobtylerwalls · 2025-01-16T14:48:55Z

arches_lingo/serializers.py

+                label_language = label.appellative_status_ascribed_name_language[0]
+                label_type = label.appellative_status_ascribed_relation[0]


These fail with KeyError if not provided. If these fields are truly required at the node config level (seems like a model issue that they aren't) then the arches queryset layer that handles the tile validation logic will reject it. But since that layer is later, this layer needs to be a little more forgiving.

Unless!

We "override" the fields in this serializer to enforce that they're required at the outermost layer. Something like:

diff --git a/arches_lingo/serializers.py b/arches_lingo/serializers.py index 1017492..db61b2b 100644 --- a/arches_lingo/serializers.py +++ b/arches_lingo/serializers.py @@ -1,3 +1,4 @@ +from rest_framework import serializers from rest_framework.exceptions import ValidationError from arches_references.models import ListItem @@ -47,6 +48,10 @@ class SchemeLabelSerializer(ArchesModelSerializer): class SchemeLabelTileSerializer(ArchesTileSerializer): + # Get this from a mapping from datatypes to serializer fields + # then add required=True to fail faster + appellative_status_ascribed_name_language = serializers.JSONField(allow_null=True, required=True) + class Meta: model = TileModel graph_slug = "scheme"

If we do that, then your business logic doesn't need to guard against missing values, because we fail before getting here (Looks like we need to improve the error toast to actually return the error message from the server keyed on field name instead of just "Bad Request".)

Nice to have some options here. I'm not convinced overriding the serializer field is better; it's probably easier to just fix your [0] 👍

Added a fix for this, but we probably want a dataclass or something to prevent this awkward double fallback.

jacobtylerwalls · 2025-01-16T14:51:57Z

arches_lingo/serializers.py

+    def validate(self, data):
+        data = super().validate(data)
+        graph_slug = self.Meta.graph_slug
+        PREF_LABEL_LIST_ITEM = ListItem.objects.get(list_item_values__value="prefLabel")


Need to catch MultipleObjectsReturned or otherwise define which list item wins.

Pushed an update for this, a bit silly, but there are probably better things we can do in the future around identifying a list with a natural key (should name be unique, assuming it's not localized?)

jacobtylerwalls · 2025-01-16T16:07:51Z

arches_lingo/serializers.py

+            resource_instance_id = self.instance.resourceinstance.pk
+            resource_instance = self.instance.resourceinstance.__class__.as_model(
+                graph_slug, resource_ids=[resource_instance_id]
+            ).get(pk=resource_instance_id)


Done! I think you and I were only testing updates, not creates. I also added a little developer convenience for now.

jacobtylerwalls · 2025-01-16T16:08:32Z

arches_lingo/serializers.py

+                    and label_language["uri"] == new_label_language["uri"]
+                ):
+                    raise ValidationError(
+                        "A preferred label with the same language already exists for this scheme."


I localized this and borrowed the wording from arches-references to prevent creating a second translatable string.

jacobtylerwalls · 2025-01-16T16:21:00Z

arches_lingo/src/arches_lingo/components/generic/LabelEditor.vue

+const router = useRouter();
+const selectedLanguage = inject(selectedLanguageKey) as Ref<Language>;
+const { $gettext } = useGettext();
+const formId = (route.params.id as string) ?? Math.random().toString();


TIL that Vue 3.5 ships a useId() hook for this: https://vuejs.org/api/composition-api-helpers.html#useid

That's likely better than rolling our own.

Also: why wouldn't we put this label inside the <DateDatatypeEditor> and perhaps use <label for= like you had before, instead of passing this down through all these layers?

Pushed an update to use this

This reverts commit 510f59c.

jacobtylerwalls · 2025-01-17T23:11:41Z

I'm noticing that I still can't add new labels, and the dropdowns powering the editor still do not show their selection if creating a new label.

I fixed this by reverting the revert @johnatawnclementawn added after the demo. Johnathan, let me know if there's a reason you wanted to revert and let's discuss 👍

Also I'm noticing this warning in the console now

Fixed in 35a1237

johnatawnclementawn linked an issue Dec 31, 2024 that may be closed by this pull request

Scheme Labels Form/Editor #147

Closed

chrabyrd reviewed Jan 2, 2025

View reviewed changes

aarongundel force-pushed the adg/146-scheme-names branch 2 times, most recently from 3f68847 to fa5dc08 Compare January 3, 2025 17:16

Base automatically changed from adg/146-scheme-names to main January 3, 2025 17:16

johnatawnclementawn force-pushed the jmc/147_scheme_label_editor branch from 10b4718 to 7e3d631 Compare January 3, 2025 20:24

johnatawnclementawn marked this pull request as ready for review January 8, 2025 16:04

chrabyrd approved these changes Jan 8, 2025

View reviewed changes

chrabyrd requested changes Jan 8, 2025

View reviewed changes

jacobtylerwalls reviewed Jan 8, 2025

View reviewed changes

johnatawnclementawn and others added 18 commits January 8, 2025 15:20

make import flexible

51ca818

Stub out reference dt widget rename from controlledlistitem

0a6e33b

Stub out label viewer #147

b98ed49

Parse refdt options and pass to widget component #147

c2093dd

Get option labels to populate dropdown & align listitem type with til…

6c10a1f

…e config #146

Update uri ref for select component reactivity #147

9ff15fa

Handle multiValue reference datatype in widget editor #147

310401c

Use refdt widget in label editor #147

6b74f41

nits #147

abf06f7

Add resource instance nodes to scheme label form #147

e3c119d

Add person_rdm slug and upgrade model to latest v8 requirements #147

5d8c7c3

pr feedback & readability improvements #147

60536ab

nit #147

2a2b889

Save / update scheme labels #147

20227b0

creates date datatype after rebase #147 (#174)

faae95c

nit #147

1f6b076

Parse date value from primevue #147

4ea7d6c

Handle value updates for all node types #147

f067d49

johnatawnclementawn and others added 2 commits January 8, 2025 15:22

nit #147

f901175

Fix console warning about shallowRef

c290ff6

johnatawnclementawn force-pushed the jmc/147_scheme_label_editor branch from b03e64d to c290ff6 Compare January 8, 2025 20:23

jacobtylerwalls and others added 11 commits January 8, 2025 15:39

Fix reactivity of primeuve picker

d325db5

ensure metastring viewer is in view mode #147

a3c7cb4

Merge branch 'main' into jmc/147_scheme_label_editor

86f230e

nit

c07c6ae

Revert "Fix reactivity of primeuve picker"

510f59c

This reverts commit d325db5.

pr feedback #147

65f5e83

Add form labels and passthru values for accessibility #147

e41c50f

When creating new tile, update edit session #147

8ffb6c1

nit #147

557a2a0

Prevent multiple prefLabels in same language #147

f71bacb

nit #147

8418e8a

johnatawnclementawn mentioned this pull request Jan 15, 2025

Add ability to filter controlled_lists endpoint archesproject/arches-references#57

Closed

jacobtylerwalls reviewed Jan 15, 2025

View reviewed changes

chrabyrd approved these changes Jan 16, 2025

View reviewed changes

johnatawnclementawn added 2 commits January 15, 2025 20:10

Avoid hard coded fetching of reference dt options #147

96dd5b2

naming nit #147

02a721e

johnatawnclementawn mentioned this pull request Jan 16, 2025

Scheme Label form follow-up #184

Open

Harden prefLabel check to prevent tile creation errors

600fe2f

jacobtylerwalls reviewed Jan 16, 2025

View reviewed changes

jacobtylerwalls self-assigned this Jan 17, 2025

jacobtylerwalls added 4 commits January 17, 2025 17:53

More informative failure (for now) if multiple prefLabel list items

46e44f1

Use useId()

bff6057

Revert "Revert "Fix reactivity of primeuve picker""

57ea8cc

This reverts commit 510f59c.

Fix pt pass thru

35a1237

jacobtylerwalls merged commit ed3b0ca into main Jan 17, 2025
4 checks passed

jacobtylerwalls deleted the jmc/147_scheme_label_editor branch January 17, 2025 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheme Label Editor & Reference Datatype Widget #169

Scheme Label Editor & Reference Datatype Widget #169

johnatawnclementawn commented Dec 31, 2024 •

edited

Loading

chrabyrd left a comment

chrabyrd left a comment

chrabyrd left a comment

chrabyrd left a comment •

edited

Loading

jacobtylerwalls left a comment

robgaston commented Jan 8, 2025

jacobtylerwalls Jan 15, 2025

jacobtylerwalls Jan 15, 2025

johnatawnclementawn Jan 16, 2025

jacobtylerwalls Jan 16, 2025

chrabyrd left a comment

chrabyrd Jan 15, 2025

johnatawnclementawn Jan 16, 2025

chrabyrd Jan 15, 2025

johnatawnclementawn Jan 16, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 17, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 16, 2025

jacobtylerwalls Jan 17, 2025

jacobtylerwalls commented Jan 17, 2025

		label_language = label.appellative_status_ascribed_name_language[0]
		label_type = label.appellative_status_ascribed_relation[0]

Scheme Label Editor & Reference Datatype Widget #169

Scheme Label Editor & Reference Datatype Widget #169

Conversation

johnatawnclementawn commented Dec 31, 2024 • edited Loading

chrabyrd left a comment

Choose a reason for hiding this comment

chrabyrd left a comment

Choose a reason for hiding this comment

chrabyrd left a comment

Choose a reason for hiding this comment

chrabyrd left a comment • edited Loading

Choose a reason for hiding this comment

jacobtylerwalls left a comment

Choose a reason for hiding this comment

robgaston commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrabyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls commented Jan 17, 2025

johnatawnclementawn commented Dec 31, 2024 •

edited

Loading

chrabyrd left a comment •

edited

Loading