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
fix(site): fix floating number on duration fields #13209
Conversation
Related: #13071 Assuming this will interpret units when the page loads? Just wondering how it looks if I change the value via CLI. Can we add a tooltip when "Days" is greyed out explaining why it's blocked?
|
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.
I think this looks good so far!
It actually reminds me of the component I built out for my Coder take-home, though obviously, using MUI simplifies the code a lot more compared to using HTML input
s
@stirby Yeah, the way the component is set up right now is that it will default to |
@Parkreiner thanks for the review. Did you have time to play around with it in the storybook? I'm going to create tests for it after being sure the UX is good enough. |
@Parkreiner after implementing this component into the form I realized you were right about two things:
|
@Parkreiner I updated the code a bit and would love a second round of review. @stirby I would appreciate it if you could QA the "Time until dormant" field experience and let me know if it is an improvement compared to what we have. A quick demo: Screen.Recording.2024-05-09.at.14.10.41.mov |
✔️ PR 13209 Updated successfully.
|
@BrunoQuaresma Have a couple of things to wrap up today, but at the very latest, I'll do a real round of review tomorrow morning |
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.
I think this looks really good!
I don't know how much more you need to update the code to get the failing tests to pass, but if you think they just need a few tiny tweaks, I can go ahead and approve, just to make sure you're not blocked
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.
Okay, cool – approving
@BrunoQuaresma, few things. I like the improved error tooltip, but it seems to appear only under certain conditions: Screen.Recording.2024-05-10.at.11.58.41.AM.movIt seems like if I enter an incorrect value right after switching units, the error appears. I'm also able to sometimes apply the float values, but it uses the integer. Can we change the conditional to be more consistent? IE showing up any time there's an incorrect value? Also:
Looks great otherwise, nice work. |
@stirby I would appreciate another QA round. I updated the preview with the latest changes 🙏 |
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.
Looks good, feels good. Did you remove the ability to enter decimals? I think it's a clean solution.
Would ask that @Parkreiner takes one more look.
@stirby Yeah, I can do another once-over in about an hour |
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.
Yup, I think the component's in a good spot now
</div> | ||
|
||
{helperText && ( | ||
<FormHelperText error={props.error}>{helperText}</FormHelperText> |
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.
Was there a reason why props.error
wasn't destructured from the props at the top of the component?
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.
Because it is used by the TextField props as well.
if ( | ||
action.unit === "days" && | ||
!canConvertDurationToDays(currentDurationMs) | ||
) { | ||
return state; | ||
} |
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.
Just making sure: this is just a precaution/double-bookkeeping, right? Even though the UI has the disabled
check to prevent the days unit from being selected at certain points, the reducer is also enforcing that the state update can't go through, in case the UI is set up wrong?
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.
Yes, I think it can be useful to have it in the reducer + disabled attribute. Wdyt?
Demo about the new
<DurationField />
component.Screen.Recording.2024-05-08.at.14.11.24.mov
Storybook on Chromatic: https://624de63c6aacee003aa84340-wnakkqswca.chromatic.com/?path=/story/components-durationfield--hours