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.

Sign up for GitHub

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

Jump to bottom

Add custom input fields for editing metadata #354

Open
wants to merge 15 commits into
base: feature/metadata-editing/develop
Choose a base branch
from
Open

Add custom input fields for editing metadata #354

wants to merge 15 commits into from

Conversation

aswallace
Copy link
Contributor

@aswallace aswallace commented Dec 6, 2024 •
edited
Loading

This is a follow-up to #338 that adds custom fields for different data types based on the Figma designs.
closes #340

Description

The input type switch is modeled off the way we did it in AnnotationFilterForm and re-uses some of the same components.

  • Uses our existing choice group/text field/date field components for boolean/string/date+datetime respectively
  • Adds a new "duration" form type
  • Creates a new numerical input component to be more stylistically consistent (and replaced usage in our NumberRangePicker component)

Screenshots

Duration
new duration form
Boolean
choice group
Date
image
DateTIme
image
Dropdown
combo box
Number
new numerical input
Text/Default
default for any type

@aswallace aswallace requested review from SeanLeRoy, BrianWhitneyAI, kmitcham, pgarrison and lynwilhelm and removed request for SeanLeRoy December 6, 2024 18:41
@aswallace aswallace self-assigned this Dec 6, 2024
@aswallace aswallace linked an issue Dec 6, 2024 that may be closed by this pull request
Render input fields by annotation type #340
Open
@lynwilhelm
Copy link
Collaborator

@aswallace Great work! Just 3 feedback notes:

  1. Duration: The labels are overlapping the inputs. There should be a couple pixels of space in between.
  2. Hint text should be consistent with pattern, which is to describe what the user should do using an action word. For example "Type an option name..." So, Text/default should read "Type in value(s)" and Number should say "Type a numerical value..."
  3. Can the little calendar be the same color as text so it doesn't look disabled?

Thanks for sharing!

@aswallace
Copy link
Contributor Author

aswallace commented Dec 6, 2024 •
edited
Loading

Thank you @lynwilhelm! Addressed feedback in f13faee and updated screenshots to reflect changes

pgarrison
pgarrison approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@pgarrison pgarrison left a comment

Choose a reason for hiding this comment

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

This UI looks so nice!

packages/core/components/EditMetadata/MetadataDetails.module.css Outdated Show resolved Hide resolved
packages/core/components/NumberRangePicker/NumberField.module.css Outdated Show resolved Hide resolved
packages/core/entity/AnnotationFormatter/duration-formatter.ts Outdated

return display.trim();
},

valueOf(value: any) {
const RANGE_OPERATOR_REGEX = /([0-9]+)D ([0-9]+)H ([0-9]+)M ([0-9]*\.?[0-9]+)S/g;
const exec = RANGE_OPERATOR_REGEX.exec(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like matches is more clear than exec?

Copy link
Contributor Author

@aswallace aswallace Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fixed in 43ddfec

packages/core/components/NumberRangePicker/NumberField.tsx Show resolved Hide resolved
packages/core/components/DurationForm/index.tsx Outdated Show resolved Hide resolved
@aswallace aswallace force-pushed the feature/metadata-editing/develop branch from 861e40b to fd404ae Compare December 18, 2024 19:51
@aswallace aswallace force-pushed the feature/metadata-editing/custom-input-fields branch from 761fa46 to 43ddfec Compare December 18, 2024 19:58
SeanLeRoy
SeanLeRoy reviewed Dec 18, 2024
View reviewed changes
packages/core/components/ComboBox/index.tsx Outdated Show resolved Hide resolved
packages/core/components/DurationForm/index.tsx
className={styles.inputField}
id="durationDays"
label="Days"
onChange={(event) => setDurationDays(event?.target?.value || "")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this component could exclusively pass around Number type values (for cases with leading zeroes it would just not yet return a value AKA undefined); NBD to keep this though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in theory, but running into some implementation bugs

packages/core/components/EditMetadata/EditMetadata.module.css Outdated Show resolved Hide resolved
packages/core/components/EditMetadata/MetadataDetails.tsx
const inputField = () => {
switch (props.fieldType) {
case AnnotationType.DATE:
case AnnotationType.DATETIME:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no time selector for DATETIME types or is there always a time selector for DATE types? Either of those would be bad no?

Copy link
Contributor Author

@aswallace aswallace Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately yes, the date picker does not provide a time selector & defaults to (I think) 8am PT. This issue also exists in the AnnotationFilterForm.
edit: I just found a timepicker component in fluent, so I'll see about adding that in
edit2: it's super buggy, if I can't come up with an alternative by eod I'll open a ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 9328a44
image

@aswallace @SeanLeRoy
fix double semicolon
c47206a 
Co-authored-by: Sean LeRoy <[email protected]>
@aswallace @SeanLeRoy
remove bool assignment
83709f3 
Co-authored-by: Sean LeRoy <[email protected]>
@aswallace aswallace requested a review from SeanLeRoy December 21, 2024 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgarrison pgarrison pgarrison approved these changes

@kmitcham kmitcham kmitcham left review comments

@BrianWhitneyAI BrianWhitneyAI Awaiting requested review from BrianWhitneyAI

@lynwilhelm lynwilhelm Awaiting requested review from lynwilhelm

@SeanLeRoy SeanLeRoy Awaiting requested review from SeanLeRoy

Assignees

@aswallace aswallace

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Render input fields by annotation type
5 participants
@aswallace @lynwilhelm @kmitcham @pgarrison @SeanLeRoy