Skip to content
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.

Sign up for GitHub

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

[LEMS-2880] Bugfix the close button focus outline on hover in keypad and other style issues #2296

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[LEMS-2880] Bugfix the close button focus outline on hover in keypad and other style issues #2296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
79b1ff3
Reinstate the focus outline of the x button in math-input component, …
ivyolamit Mar 11, 2025
391629a
update snapshots
ivyolamit Mar 11, 2025
e16dfb8
docs(changeset): [LEMS-2880] Bugfix the close button focus outline on…
ivyolamit Mar 11, 2025
49c2537
[bugfix/LEMS-2880-reinstate-focus-outline] Handle close keypad
ivyolamit Mar 11, 2025
bd4dd02
[bugfix/LEMS-2880-reinstate-focus-outline] Merge remote-tracking bran…
ivyolamit Mar 11, 2025
2f7a21e
[bugfix/LEMS-2880-reinstate-focus-outline] Merge remote-tracking bran…
ivyolamit Mar 12, 2025
fc048e3
[bugfix/LEMS-2880-reinstate-focus-outline] updated snapshots
ivyolamit Mar 12, 2025
7546900
[bugfix/LEMS-2880-reinstate-focus-outline] Merge branch 'main' into b…
ivyolamit Mar 17, 2025
206df0a
[bugfix/LEMS-2880-reinstate-focus-outline] Merge branch 'main' into b…
ivyolamit Mar 19, 2025
3caf2c7
[bugfix/LEMS-2880-reinstate-focus-outline] updated changeset
ivyolamit Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/dry-shirts-refuse.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/math-input": patch
"@khanacademy/perseus": patch
---

Fix expression widget styling issues. Close button focus outline is now visible, backspace button styling is now consistent with other buttons, and adjusted the popover padding.
4 changes: 2 additions & 2 deletions packages/math-input/src/components/keypad/__tests__/__snapshots__/keypad.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ exports[`keypad should snapshot expanded: first render 1`] = `
class="default_xu2jcg-o_O-outerBoxBase_3w5jmh"
>
<div
class="default_xu2jcg-o_O-base_muxx52-o_O-inlineStyles_wps3dh"
class="default_xu2jcg-o_O-base_muxx52-o_O-inlineStyles_s65xgl"
>
<svg
fill="none"
Expand Down Expand Up @@ -1891,7 +1891,7 @@ exports[`keypad should snapshot unexpanded: first render 1`] = `
class="default_xu2jcg-o_O-outerBoxBase_3w5jmh"
>
<div
class="default_xu2jcg-o_O-base_muxx52-o_O-inlineStyles_wps3dh"
class="default_xu2jcg-o_O-base_muxx52-o_O-inlineStyles_s65xgl"
>
<svg
fill="none"
Expand Down
2 changes: 1 addition & 1 deletion packages/math-input/src/components/keypad/keypad-mathquill.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export function V2KeypadWithMathquill() {
content={
<PopoverContentCore
style={{
padding: 10,
padding: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given this is in a Story... did you also test this fix in webapp? Just want to make sure we aren't fixing this style issue for a story only.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good call! I'll do that. Base on code search we might also need to update the webapp side. Let me get back to you on this after i'm fully setup with the webapp and have a separate PR for it.

maxWidth: "initial",
}}
>
Expand Down
2 changes: 1 addition & 1 deletion packages/math-input/src/components/keypad/shared-keys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export default function SharedKeys(props: Props) {
keyConfig={Keys.BACKSPACE}
onClickKey={onClickKey}
coord={[5, 3]}
action
secondary
/>
</>
);
Expand Down
12 changes: 7 additions & 5 deletions packages/perseus/src/components/math-input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,13 @@ class InnerMathInput extends React.Component<InnerProps, State> {
// a keyboard event is "keydown" type.
// In react without WonderBlocks, "enter" or "space" keydown events
// are also "click" events, differentiated by "detail".
if (e.type === "click") {
if (e?.type === "click") {
this.focus();
}

if (key === "DISMISS") {
this.closeKeypad();
}
};

render(): React.ReactNode {
Expand Down Expand Up @@ -340,10 +344,9 @@ class InnerMathInput extends React.Component<InnerProps, State> {
onBlur={() => this.blur()}
/>
<Popover
rootBoundary="document"
opened={this.state.keypadOpen}
onClose={() => this.closeKeypad()}
Copy link
Member

Choose a reason for hiding this comment

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

Interesting why remove the onClose event? How was this effecting the style of the close button?

Copy link
Author

Choose a reason for hiding this comment

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

onClose event was remove and used the showDismiss props of the keypad component. The onClose event is something that the Popover component handles which is the outer component which has a different position of the close button, but by leveraging the showDismiss props it will correctly align in the TabBar which the icons are position as part of the inner component.

dismissEnabled
rootBoundary="document"
aria-label={this.context.strings.mathInputTitle}
aria-describedby={`popover-content-${popoverContentUniqueId}`}
content={() => (
Expand All @@ -355,7 +358,6 @@ class InnerMathInput extends React.Component<InnerProps, State> {
{this.context.strings.mathInputDescription}
</HeadingMedium>
<PopoverContentCore
closeButtonVisible
style={styles.popoverContent}
>
<DesktopKeypad
Expand All @@ -372,6 +374,7 @@ class InnerMathInput extends React.Component<InnerProps, State> {
mapButtonSets(
this.props?.buttonSets,
))}
showDismiss
/>
</PopoverContentCore>
</>
Expand Down Expand Up @@ -554,7 +557,6 @@ const styles = StyleSheet.create({
},
popoverContent: {
padding: 0,
paddingBottom: spacing.xxSmall_6,
maxWidth: "initial",
},
});
Expand Down
Loading