-
Notifications
You must be signed in to change notification settings - Fork 350
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
Support visible/aria label for the Expression widget #1404
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (66f1682) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1404 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1404 |
Size Change: -4.46 kB (-0.52%) Total Size: 850 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1404 +/- ##
==========================================
+ Coverage 69.86% 70.37% +0.50%
==========================================
Files 505 509 +4
Lines 105665 105014 -651
Branches 7670 11369 +3699
==========================================
+ Hits 73826 73901 +75
+ Misses 31659 31113 -546
+ Partials 180 0 -180
... and 178 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
}; | ||
}, | ||
); | ||
this.mathField?.setAriaLabel(ariaLabel); |
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.
Aria-label for mobile
@@ -241,6 +241,7 @@ class InnerMathInput extends React.Component<InnerProps, State> { | |||
); | |||
} | |||
|
|||
this.__mathField?.setAriaLabel(this.props.ariaLabel); |
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.
Aria-label for desktop
@@ -327,243 +328,278 @@ describe("Expression Widget", function () { | |||
await assertCorrect(userEvent, item, "tg x"); | |||
}); | |||
}); | |||
}); |
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.
A lot of these changes are because I thought some of these describes should have been nested. It might be easier to look at this file with "hide whitespace" enabled.
/> | ||
<View style={{padding: "15px 4px 0"}}> | ||
{!!this.props.visibleLabel && ( | ||
<label |
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.
Mobile visible label
{/** | ||
<View style={{margin: "5px 5px 0"}}> | ||
{!!this.props.visibleLabel && ( | ||
<label |
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.
Desktop visible label
noob question: when do the new labels get saved and pushed to the backend? |
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's getting so close! The biggest questions I have is whether we can use a WonderBlocks components for the labels so the styling is consistent with our design system.
packages/perseus-editor/src/__stories__/article-editor.stories.tsx
Outdated
Show resolved
Hide resolved
); | ||
} | ||
} | ||
|
||
const styles = StyleSheet.create({ |
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.
Did Caitlyn get a chance to take a look at these padding values for mobile and desktop?
Summary:
Adds two new options for the Expression widget via the ExpressionEditor:
Caitlyn wanted these to be separate so that they could read differently. Her example is if the label should be "Number in cm": a screen reader might read that as "Number in C M" whereas we want it to read "Number in centimeters." So this supports that case.
Issue: LEMS-2081
Test plan:
Looking at the labels:
Editing the labels: