-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: develop
Are you sure you want to change the base?
Feature/corpus form #1659
Conversation
Also offer a reset sample
There was a problem hiding this 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.
frontend/src/app/corpus-definitions/definitions-overview/definitions-overview.component.html
Show resolved
Hide resolved
frontend/src/app/corpus-definitions/form/field-form/field-form.component.html
Outdated
Show resolved
Hide resolved
frontend/src/app/corpus-definitions/form/field-form/field-form.component.html
Outdated
Show resolved
Hide resolved
frontend/src/app/corpus-definitions/form/meta-form/meta-form.component.html
Outdated
Show resolved
Hide resolved
<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> |
There was a problem hiding this comment.
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.
frontend/src/app/corpus-definitions/form/meta-form/meta-form.component.html
Show resolved
Hide resolved
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. |
No description provided.