Add support for centered NumericInputs#3731
Conversation
|
Size Change: +400 B (+0.08%) Total Size: 508 kB 📦 View Changed
ℹ️ View Unchanged
|
| }); | ||
| }); | ||
|
|
||
| describe("textAlign", () => { |
There was a problem hiding this comment.
Tests to make sure:
- We can migrate from
rightAligntotextAlign - We can parse
textAligncorrectly
| }), | ||
| ); | ||
|
|
||
| function migrateV0ToV1( |
There was a problem hiding this comment.
Replacing rightAlign with textAlign is a major change and requires a migration.
| coefficient: false, | ||
| labelText: "", | ||
| size: "normal", | ||
| textAlign: "center", |
There was a problem hiding this comment.
Test to make sure we can parse the new textAlign
| ]); | ||
| }); | ||
|
|
||
| it("converts alignment when rightAlign is undefined", () => { |
There was a problem hiding this comment.
Make sure IN-to-NI still works.
There was a problem hiding this comment.
Greatly appreciate you adding these tests for us!
| }); | ||
|
|
||
| it("should be possible to select right alignment", async () => { | ||
| it("should be possible to change text alignment", async () => { |
There was a problem hiding this comment.
Test to make sure the editor can select the new options
| }; | ||
|
|
||
| // Verifies the center-aligned text input variant with a pre-filled value — the value should appear in the center | ||
| export const CenterTextAlign: Story = { |
There was a problem hiding this comment.
Visual regression test for centered NIs.
| "coefficient": false, | ||
| "labelText": "", | ||
| "size": "normal", | ||
| "textAlign": "center", |
There was a problem hiding this comment.
This is the important bit here.
| "coefficient": false, | ||
| "labelText": "", | ||
| "size": "normal", | ||
| "textAlign": "end", |
There was a problem hiding this comment.
This is the expected transformation of the legacy schema: rightAlign: true -> textAlign: end.
…t alignment in NumericInputs
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (d550a0d) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3731If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3731If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3731 |
| coefficient: false, | ||
| labelText: "", | ||
| size: "normal", | ||
| rightAlign: true, |
There was a problem hiding this comment.
Test to make sure we can still parse rightAlign
| * How to align the text in the input | ||
| * it's "start"/"end" vs "left"/"right" to support i18n in the future | ||
| */ | ||
| textAlign: "start" | "end" | "center"; |
There was a problem hiding this comment.
I could have done textAlign? but I feel like it's easier to just let the parser provide a default so we always have a value.
There was a problem hiding this comment.
I like the idea of working to reduce optionality in the schema and have the parser fill in details. I think we found that depending on the React widget components do the defaulting is tricky now that we have server-side scoring, etc.
jeremywiebe
left a comment
There was a problem hiding this comment.
Drive-by comments. I'll leave it to project folks to approve. Thanks for this Matthew!
| expect(result.value.options.textAlign).toBe("start"); | ||
| }); | ||
|
|
||
| it("migrates from v0 to v1 when textAlign is true", () => { |
There was a problem hiding this comment.
| it("migrates from v0 to v1 when textAlign is true", () => { | |
| it("migrates from v0 to v1 when rightAlign is true", () => { |
| * How to align the text in the input | ||
| * it's "start"/"end" vs "left"/"right" to support i18n in the future | ||
| */ | ||
| textAlign: "start" | "end" | "center"; |
There was a problem hiding this comment.
I like the idea of working to reduce optionality in the schema and have the parser fill in details. I think we found that depending on the React widget components do the defaulting is tricky now that we have server-side scoring, etc.
| const textAlign = ( | ||
| <label> | ||
| Text alignment | ||
| <SingleSelect | ||
| selectedValue={this.props.textAlign} | ||
| onChange={(value) => { | ||
| this.props.onChange({textAlign: value}); | ||
| }} | ||
| placeholder="Select text alignment" | ||
| > | ||
| Right | ||
| </Pill> | ||
| </fieldset> | ||
| <OptionItem value="start" label="Left" /> | ||
| <OptionItem value="center" label="Center" /> | ||
| <OptionItem value="end" label="Right" /> | ||
| </SingleSelect> | ||
| </label> |
There was a problem hiding this comment.
Not directly related, but I'd love to move away from creating variables with snippets of JSX which we then render at some place in favour of bespoke little components.
Although we don't use the React Component view much, modelling these as react components helps them show up much better in that component debugger.
I also think it's a bit more idiomatic as React can manage re-rendering at that level.
🤷♂️
Something like this:
function AlignmentSelector(props: {
value: "left" | "right" | "centre",
onChange: (alignment: string) => void,
}) {
return (<label>
Text alignment
<SingleSelect
selectedValue={this.props.textAlign}
onChange={(value) => {
this.props.onChange({textAlign: value});
}}
placeholder="Select text alignment"
>
<OptionItem value="start" label="Left" />
<OptionItem value="center" label="Center" />
<OptionItem value="end" label="Right" />
</SingleSelect>
</label>)
}And then ...
<AlignmentSelector
value={this.props.textAlign}
onChange={(alignment) => {
this.props.onChange({textAlign: alignment});
}
/>| #answercontent input[type="text"].perseus-input-right-align, | ||
| #answercontent input[type="number"].perseus-input-right-align, | ||
| .framework-perseus input[type="text"].perseus-input-right-align, | ||
| .framework-perseus input[type="number"].perseus-input-right-align { | ||
| text-align: right; | ||
| } | ||
| .framework-perseus.perseus-mobile .perseus-input-right-align .keypad-input { | ||
| text-align: right; | ||
| } |
There was a problem hiding this comment.
Do these need to be updated? Like .perseus-input-end-align or something?
| static defaultProps: DefaultProps = { | ||
| size: "normal", | ||
| rightAlign: false, | ||
| textAlign: "start", |
There was a problem hiding this comment.
If the widget options don't make this optional, can this prop be made required here and avoid needing a default?
| padding: 0.4rem; | ||
| } | ||
|
|
||
| .input-with-examples.right-align { |
There was a problem hiding this comment.
Is this .end-align now?
SonicScrewdriver
left a comment
There was a problem hiding this comment.
This generally looks great to me! I just had a comment for numeric-input.stories.tsx, and there's a couple things Jeremy pointed out that we should fix up with the styles. (I'm not sure if IN/NI widgets share any styles with the Expression widget, but hopefully not.)
| coefficient: false, | ||
| userInput: {currentValue: ""}, | ||
| rightAlign: false, | ||
| alignment: "start", |
There was a problem hiding this comment.
Should this be textAlign?
Summary:
Go schema change: https://github.com/Khan/webapp/pull/39970
This PR adds support for centered NumericInputs. This required a change to the schema for NumericInput's widgetOptions:
This meant that I needed to update the widget, editor, parser, stories, and tests. Ultimately it's a small change to the UI and a lot of changes to our safety nets.
I decided to go with
start/endvsleft/rightto set us up for i18n in the future. However maybe we want to keepleft/rightand we can addstart/endlater? That would let content creators pick: "this needs to be on the left" or "this can be at the start of whatever the locale wants".Issue: LEMS-4144
Test plan:
Most of this PR is adding/updating tests including:
?path=/story/widgets-numeric-input-visual-regression-tests-initial-state--center-text-align?path=/docs/widgets-numeric-input--docs