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

Feature/corpus form #1659

Open
wants to merge 53 commits into
base: develop
Choose a base branch
from
Open

Feature/corpus form #1659

wants to merge 53 commits into from

Conversation

JeltevanBoheemen
Copy link
Contributor

No description provided.

@lukavdplas lukavdplas marked this pull request as ready for review November 7, 2024 11:20
@lukavdplas lukavdplas self-requested a review November 7, 2024 11:21
Copy link
Contributor

@lukavdplas lukavdplas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! None of my comments are hard requirements, but some of the minor technical points would be better to resolve before we merge.

Some of my comments are larger suggestions - we can move those into separate issues.

backend/addcorpus/models.py Outdated Show resolved Hide resolved
backend/addcorpus/utils.py Outdated Show resolved Hide resolved
frontend/src/app/services/api.service.ts Outdated Show resolved Hide resolved
frontend/src/app/corpus-definitions/form/constants.ts Outdated Show resolved Hide resolved
Comment on lines 27 to 39
<div class="field is-grouped">
<div formGroupName="date_range">
<label class="label" for="min">Start date</label>
<div class="control">
<input class="input" type="date" name="min" formControlName="min" />
</div>
<label class="label" for="max">End date</label>
<div class="control">
<input class="input" type="date" name="max" formControlName="max" />
</div>
<p class="help">Enter the earliest and latest date in your corpus in YYYY-MM-DD format</p>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only use the year, you could make this a numeric control and just add January 1st for the min date, and December 31st for the max date.

In the help text, we could also mention that this doesn't need to be an exact year, it's just an indication for users.

@lukavdplas
Copy link
Contributor

Something that occurred to me: the corpus fields in the form are based on the CSV columns, but they're not displaying which field maps to which column. This will get especially confusing if you rename the fields.

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

Successfully merging this pull request may close these issues.

Upload CSV example file Form for entering corpus definitions (first draft)
2 participants