-
Notifications
You must be signed in to change notification settings - Fork 344
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
[OPIK-1020] feedback score reason #1403
base: main
Are you sure you want to change the base?
Conversation
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.
Good work, some small improvements in the code are listed.
- Maybe, we should add reasons to Experiment Item Sidebar as well

- It's weirdly bouncing when you edit an already existing reason, stop typing - it updates the state - and the reason icon dissapears and appears again. So, maybe we could avoid having it - caching the previous state
@@ -119,3 +119,17 @@ export const extractIdFromLocation = (location: string) => | |||
|
|||
export const formatNumericData = (value: number, precision = 3) => | |||
String(round(value, precision)); | |||
|
|||
export const updateTextAreaHeight = ( |
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.
it doesn't have to be specifically textArea in this function, any html element would fit. however, as we're using it only for textAreas, it's ok to keep it now
return { | ||
value: inputValue, | ||
onChange: handleInputChange, | ||
}; |
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.
as an idea, I like that sometimes in libraries they return [value, onChange]
. So, a person could you use it as const [debouncedValue, debouncedChangeValue] = useDebouncedValue({ ... })
. your solution is good as well :)
Details
Issues
Resolves #
Testing
Documentation