Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix(site): fix floating number on duration fields#13209

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
BrunoQuaresma merged 18 commits intomainfrombq/fix-float-input
May 17, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma commentedMay 8, 2024•
edited
Loading

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

@BrunoQuaresmaBrunoQuaresma self-assigned thisMay 8, 2024
@stirby
Copy link
Collaborator

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?

"Set duration to a multiple of 24 hours to change units."

BrunoQuaresma reacted with thumbs up emoji

Copy link
Member

@ParkreinerParkreiner left a 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 HTMLinputs

@Parkreiner
Copy link
Member

Parkreiner commentedMay 8, 2024•
edited
Loading

@stirby Yeah, the way the component is set up right now is that it will default todays whenever the numeric value originally passed in is cleanly divisible by 24 hours. Otherwise, it uses hours

stirby reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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.

@BrunoQuaresma
Copy link
CollaboratorAuthor

@Parkreiner after implementing this component into the form I realized you were right about two things:

  • It is better to only have the value as a number instead of having to deal withundefined.
  • We need to handle external value updates to avoid a wrong state in the time unit.
    🙏

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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

@github-actionsGitHub Actions
Copy link

github-actionsbot commentedMay 9, 2024•
edited
Loading

✔️ PR 13209 Updated successfully.
🚀 Access the credentialshere.

cc:@BrunoQuaresma

github-actions[bot] reacted with rocket emoji

@Parkreiner
Copy link
Member

@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

BrunoQuaresma reacted with thumbs up emoji

Copy link
Member

@ParkreinerParkreiner left a 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

Copy link
Member

@ParkreinerParkreiner left a comment

Choose a reason for hiding this comment

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

Okay, cool – approving

@stirby
Copy link
Collaborator

@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.mov

It 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:

  • Should we make the entry box fill the width like the old one?
  • Could we apply it to the other options?

ui_component_dormancy

Looks great otherwise, nice work.

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@stirby I would appreciate another QA round. I updated the preview with the latest changes 🙏

Copy link
Collaborator

@stirbystirby left a 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.

@Parkreiner
Copy link
Member

@stirby Yeah, I can do another once-over in about an hour

Copy link
Member

@ParkreinerParkreiner left a 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>
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason whyprops.error wasn't destructured from the props at the top of the component?

Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaMay 17, 2024•
edited
Loading

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.

Comment on lines +48 to +53
if (
action.unit === "days" &&
!canConvertDurationToDays(currentDurationMs)
) {
return state;
}
Copy link
Member

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 thedisabled 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?

Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaMay 17, 2024•
edited
Loading

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?

@BrunoQuaresmaBrunoQuaresma marked this pull request as ready for reviewMay 17, 2024 18:25
@BrunoQuaresmaBrunoQuaresma merged commit4af0f09 intomainMay 17, 2024
28 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/fix-float-input branchMay 17, 2024 18:26
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@ParkreinerParkreinerParkreiner approved these changes

@stirbystirbystirby approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@BrunoQuaresma@stirby@Parkreiner
[8]ページ先頭
©2009-2025 Movatter.jp