Adding calculator inputs #2343
Adding calculator inputs #2343
Conversation
🦋 Changeset detectedLatest commit: 23a61c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2343 +/- ##
==========================================
+ Coverage 71.63% 71.72% +0.09%
==========================================
Files 111 113 +2
Lines 5704 5811 +107
Branches 1091 1117 +26
==========================================
+ Hits 4086 4168 +82
- Misses 1611 1636 +25
Partials 7 7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
If this is a ~last big change to the calculators functionality in the next few weeks/months then I'm fine with merging this.
If you plan to add more complexity, i.e. field layouts, or something else that would require adding even more moving parts, then I'd prefer to make a pause here for refactoring, or do that refactoring first, to make things more manageable (not a problem if we don't delay this PR for this, and this seems solid enough for 0.8.6 release).
Specific cleanups I have in mind:
- unifying input components (as I mentioned in one of the comments)
- RHF
- cleaning up the reducer part, as I hoped to do earlier during the first Calculators PR
| input: (base) => ({ | ||
| ...base, | ||
| "input:focus": { | ||
| boxShadow: "none", |
There was a problem hiding this comment.
I'm not sure what this does, is it a fix for this blue rectangle?
At least, I see it on https://quri-ui.vercel.app/?path=/docs/ui-forms-selectstringformfield--docs but not on https://quri-ui-git-calculator-inputs-quantified-uncertainty.vercel.app/?path=/docs/ui-forms-selectstringformfield--docs, so that's my best guess.
There was a problem hiding this comment.
Yep, that's to fix the blue rectangles.
| value: option, | ||
| label: option, | ||
| }))} | ||
| styles={{ |
There was a problem hiding this comment.
Several comments:
- There's probably nothing calculator-specific here, so this all belongs in
StyledSelectorSelectFormField, as you mention in the description - I'm not sure what all these styles do, but some of them make things worse, e.g. text is not centered:
(compare with https://quri-ui-git-calculator-inputs-quantified-uncertainty.vercel.app/?path=/story/ui-forms-selectstringformfield--default)
- In my impression this is getting complex enough that RHF would be useful, even if type-safety features of *Field components won't be available (because calculator fields are dynamic). But it's fine if you don't want to deal with it now.
There was a problem hiding this comment.
Can you give me some tips for making a StyledSelect? I'm not sure how to easily type all of the potential props (there are many).
I'm really only changing the styles, so it seems like maybe I could just export a variable for those styles, like,
StyledSelectStyles = {{input: ...}}
Or could you do some attempt here? Some of the code in the UI around this is fairly gnarly, mainly with the type system.
There was a problem hiding this comment.
The main thing the styles do is make this input consistent with other inputs; use purple instead of blue. It makes this "small", and this causes the non-centering. I'll improve this later on.
If you plan to add more complexity, i.e. field layouts, or something else that would require adding even more moving parts, then I'd prefer to make a pause here for refactoring, or do that refactoring first, to make things more manageable (not a problem if we don't delay this PR for this, and this seems solid enough for 0.8.6 release). I don't have many changes really planned. Haven't thought much about field layouts yet - have no active proposal in mind. I could definitely imagine adding another 1-3 input types. "TextWithUnit" is the main one I could see being tricky. "Slider" might accept a number instead of a string. |
Yeah, scaling input types horizontally is not a problem, I meant architectural complexity. Though some input types could require that, but "number" thing seems similar to "select is a string which needs to be quoted" thing that you did here and probably won't be hard. |
…state and effects
…s-slava-patches Calculator inputs - improvements/refactorings
Adds input types. Specifically, Text, TextArea, Checkbox, and Select. These now work in the calculator.
I think this is ready for a review. Depending on your take, this might almost be done, or might require a lot more work.
Some obvious things I'd want to spend more time on include: