Feature/frontend scaffold#17
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a frontend scaffold for the Annotator Tool by adding several Angular components and their corresponding test files. The key changes include the implementation of the KnowledgeBaseFormComponent with reactive forms, the creation of the AnnotationInputComponent that aggregates multiple form components, and the addition of supporting components such as AnnotationCommentsComponent, AnnotateComponent, and AboutComponent.
Reviewed Changes
Copilot reviewed 44 out of 49 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/annotate/annotation-input/knowledge-base-form/knowledge-base-form.component.ts | New KB form component using reactive forms. |
| frontend/src/app/annotate/annotation-input/knowledge-base-form/knowledge-base-form.component.spec.ts | Test file for the KB form component. |
| frontend/src/app/annotate/annotation-input/annotation-input.component.ts | Annotation input component integrating form components (styleUrl property issue). |
| frontend/src/app/annotate/annotation-input/annotation-input.component.spec.ts | Test for the annotation input component. |
| frontend/src/app/annotate/annotation-comments/annotation-comments.component.ts | Annotation comments component (styleUrl property issue). |
| frontend/src/app/annotate/annotation-comments/annotation-comments.component.spec.ts | Test file for the annotation comments component. |
| frontend/src/app/annotate/annotate.component.ts | Main annotate component grouping menu and navigator (styleUrl property issue). |
| frontend/src/app/annotate/annotate.component.spec.ts | Test for the annotate component. |
| frontend/src/app/about/about.component.ts | About page component (styleUrl property issue). |
| frontend/src/app/about/about.component.spec.ts | Test for the about component. |
Files not reviewed (5)
- frontend/src/app/about/about.component.html: Language not supported
- frontend/src/app/annotate/annotate.component.html: Language not supported
- frontend/src/app/annotate/annotation-comments/annotation-comments.component.html: Language not supported
- frontend/src/app/annotate/annotation-input/annotation-input.component.html: Language not supported
- frontend/src/app/annotate/annotation-input/knowledge-base-form/knowledge-base-form.component.html: Language not supported
XanderVertegaal
requested review from
Meesch and
bbonf
and removed request for
Meesch
bbonf
left a comment
bbonf
left a comment
There was a problem hiding this comment.
Sorry this took so long, I missed this PR completely.
This looks like a good basis. I personally think the input form should maybe become a separate pane that's always visible (say on the left side) and not part of the tab group, so that you could see the effect of changing the input on the parse/proof trees without switching back and forth.
| matrix: | ||
| python-version: ['3.8'] | ||
| node-version: ['18.x', '20.x'] | ||
| python-version: ['3.11'] |
There was a problem hiding this comment.
nice! maybe time to update the cookiecutter repo as well
There was a problem hiding this comment.
Good idea! I'll contact the System Team to ask about the version we usually get with new CookieCutter servers and get a PR going.
| "SUPERSET", | ||
| ] as const; | ||
|
|
||
| type KnowledgeBaseRelationship = typeof KnowledgeBaseRelationships[number]; |
There was a problem hiding this comment.
This is too advanced TypeScript for me, but how does it compare to type foo = 'qwe' | 'asd' | 'zxc' ?
There was a problem hiding this comment.
type foo = 'qwe' | 'asd' | 'zxc' gives me the correct type, but I want the type and an actual array with (only) these values, so I can use it in the template. Since I cannot derive an array from a type, I must derive a type from the array, which is what the as const and [number] do.
There was a problem hiding this comment.
Wouldn't an Enum work for that?
(I'm fine with the current solution if you add a comment, just trying to learn more)
There was a problem hiding this comment.
An enum would work perfectly fine to this end as well, and it is probably more idiomatic than using as const here. I've applied it here! 👍
There was a problem hiding this comment.
I have run into problems with enums in the past since they compile to JS in unexpected ways, leading to unwanted types for Object.values(...) (if you want to get an array of enum values). However, it seems that this is only true for numeric enums, whereas string enums behave as you would expect. I'm demonstrating the difference here:
There was a problem hiding this comment.
(In all fairness, in this case I did not make an active decision not to use enums; I simply forgot about them / did not consider them, so it is great that you mention them 😄 .)
Frontend components for the Annotator tool. Most are still empty; but for the Parser Input forms a suggestion has been made.
NB: I have updated the Node and Python versions in
test.ymlso the workflow runner uses Node 20 and Python 3.11. Not only is this what our servers will have, but it also fixes a 'test run failure' because of packages not being available for Python 3.7. Node 18 will reach end-of-life at the end of this month, so it is best to prevent warnings about that too.