Updates for namespace section #128

aarongundel · 2024-11-18T20:47:07Z

Form based editing of the namespace name (non-localized string)

chrabyrd

overall looking good! Just a few minor things 👍

chrabyrd · 2024-12-03T21:34:29Z

arches_lingo/pkg/graphs/resource_models/Scheme.json

I'm assuming Rob's ok with the model change, yeah?

we discussed this in a lingo standup, but I'll cc @robgaston just to make sure he's aware.

yup, all good!

chrabyrd · 2024-12-03T21:36:10Z

arches_lingo/src/arches_lingo/components/generic/ControlledListItem.vue

@@ -0,0 +1,22 @@
+<script lang="ts">
+const UPDATE = "update";


yeah please roll back the declaring of emit values as constants. Much rather not have the two script tags

chrabyrd · 2024-12-03T21:36:55Z

arches_lingo/src/arches_lingo/components/generic/ControlledListItem.vue

+</script>
+<template>
+    <div>
+        <div v-if="!props.mode || props.mode === 'view'">


I would, however, recommend declaring view and edit as consts

chrabyrd · 2024-12-03T21:38:30Z

arches_lingo/src/arches_lingo/components/generic/NonLocalizedString.vue

@@ -0,0 +1,27 @@
+<script lang="ts">
+const UPDATE = "update";


same. please roll back the string-as-const so there's not two script tags.

arches_lingo/src/arches_lingo/components/generic/NonLocalizedString.vue

chrabyrd · 2024-12-03T21:46:57Z

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue

@@ -17,37 +43,94 @@ const toggleSize = () => {
        emit("side");
    }
 };
+
+schemeComponents.map((x) => {


I'm typically against single-letter declarations as it makes the code harder to read.

chrabyrd · 2024-12-03T21:48:01Z

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue

@@ -17,37 +43,94 @@ const toggleSize = () => {
        emit("side");
    }
 };
+
+schemeComponents.map((x) => {
+    x.props.mode = "edit";


declare edit as a const?

chrabyrd · 2024-12-03T21:49:23Z

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue

+        props: {
+            mode: editMode,
+        },
+        on: {
+            update: onUpdated,
+        },


these may not need to be part of the component config if we expect every component to have the same props/on logic

fair, I'll keep it simple till we need otherwise.

chrabyrd · 2024-12-03T21:52:32Z

arches_lingo/src/arches_lingo/components/scheme/report/SchemeNamespace.vue

+const save = async () => {
+    await updateSchemeNamespace(
+        route.params.id as string,
+        schemeNamespace.value as SchemeNamespace,
+    );
+};
+const getSectionValue = async () => {
+    const response = await fetchSchemeNamespace(route.params.id as string);
+    schemeNamespace.value = response;
+};
+
+defineExpose({ save, getSectionValue });
+
+onMounted(async () => {
+    getSectionValue();
+});
+
+const onNamespaceNameUpdate = (val: string) => {


I'd say declare all functions with function foo() declarations, and put them on the bottom of the script tag so they're not mixed in with onMounted and defineExpose. There's an ordering of stuff in the script tag that I haven't promulgated yet, but for the time being IMO we should still be writing for readability.

fair enough

chrabyrd · 2024-12-03T21:53:42Z

arches_lingo/src/arches_lingo/constants.ts

@@ -1,5 +1,5 @@
 import type { InjectionKey, Ref } from "vue";
-import type { Language } from "@/arches_vue_utils/types";
+import type { Language } from "@/arches_vue_utils/types.ts";


chrabyrd

looking good, just a few smaller things now

chrabyrd · 2024-12-03T22:53:44Z

arches_lingo/src/arches_lingo/components/generic/ControlledListItem.vue

+const VIEW = "view";
+const EDIT = "edit";
+
+type DataComponentMode = "edit" | "view";


nit, use the declared consts

arches_lingo/src/arches_lingo/components/generic/NonLocalizedString.vue

chrabyrd · 2024-12-03T22:54:40Z

arches_lingo/src/arches_lingo/components/generic/NonLocalizedString.vue

+import NonLocalizedStringViewer from "@/arches_lingo/components/generic/NonLocalizedStringViewer.vue";
+import NonLocalizedStringEditor from "@/arches_lingo/components/generic/NonLocalizedStringEditor.vue";
+
+type DataComponentMode = "edit" | "view";


same with the declaring consts here

chrabyrd · 2024-12-03T22:58:31Z

arches_lingo/src/arches_lingo/components/scheme/report/SchemeNamespace.vue

+    mode?: DataComponentMode;
+}>();
+
+defineEmits([OPEN_EDITOR]);


ah, please revert OPEN_EDITOR to a string so there's no double-script-tag

chrabyrd · 2024-12-03T22:59:48Z

arches_lingo/src/arches_lingo/components/scheme/report/SchemeNamespace.vue

+
+<template>
+    <div>
+        <div v-if="!mode || mode === 'view'">


nit: same with declaring the string as a const here and with edit

chrabyrd

lgtm! 🚀

chrabyrd

lgtm! 🚀

aarongundel linked an issue Nov 18, 2024 that may be closed by this pull request

"Scheme Namespace" report/form section #125

Closed

aarongundel marked this pull request as draft November 19, 2024 04:09

aarongundel force-pushed the adg/100-scheme-editor-side-panel branch 2 times, most recently from 7ea336a to b141d12 Compare November 21, 2024 17:52

aarongundel force-pushed the adg/125-namespace-section branch 2 times, most recently from b1bd6b2 to c408233 Compare November 21, 2024 18:31

aarongundel force-pushed the adg/100-scheme-editor-side-panel branch from b138af1 to 2b96c94 Compare December 3, 2024 17:38

aarongundel force-pushed the adg/125-namespace-section branch 2 times, most recently from 90428f9 to bf93afd Compare December 3, 2024 18:50

aarongundel marked this pull request as ready for review December 3, 2024 18:52

aarongundel requested a review from chrabyrd December 3, 2024 18:53

aarongundel assigned chrabyrd Dec 3, 2024

chrabyrd requested changes Dec 3, 2024

View reviewed changes

aarongundel requested a review from chrabyrd December 3, 2024 22:27

aarongundel force-pushed the adg/100-scheme-editor-side-panel branch from 00ddb33 to d077e81 Compare December 3, 2024 23:00

chrabyrd requested changes Dec 3, 2024

View reviewed changes

aarongundel force-pushed the adg/125-namespace-section branch from 90eccf0 to 95a97c5 Compare December 3, 2024 23:12

Base automatically changed from adg/100-scheme-editor-side-panel to main December 3, 2024 23:13

aarongundel added 2 commits December 3, 2024 16:15

Adds namespace sec with nonlocalized string widget

0693917

pr feedback

eb1fce7

aarongundel force-pushed the adg/125-namespace-section branch from 95a97c5 to eb1fce7 Compare December 3, 2024 23:15

chrabyrd approved these changes Dec 3, 2024

View reviewed changes

pr feedback #128

309fb0e

chrabyrd approved these changes Dec 3, 2024

View reviewed changes

aarongundel merged commit 7f136e1 into main Dec 4, 2024
5 of 6 checks passed

aarongundel deleted the adg/125-namespace-section branch December 4, 2024 04:38

robgaston mentioned this pull request Dec 10, 2024

"Scheme Rights" forms/reports section #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for namespace section #128

Updates for namespace section #128

aarongundel commented Nov 18, 2024

chrabyrd left a comment

chrabyrd Dec 3, 2024

aarongundel Dec 3, 2024

robgaston Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

aarongundel Dec 3, 2024

chrabyrd Dec 3, 2024

aarongundel Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd left a comment

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd Dec 3, 2024

chrabyrd left a comment

chrabyrd left a comment

Updates for namespace section #128

Updates for namespace section #128

Conversation

aarongundel commented Nov 18, 2024

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

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

chrabyrd left a comment

Choose a reason for hiding this comment

chrabyrd left a comment

Choose a reason for hiding this comment