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

Jump to bottom

[Numeric Input] - Show "Format Options" Tooltip as List #1861

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+453 −73
Open

[Numeric Input] - Show "Format Options" Tooltip as List #1861

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
dd182db
Adjust styling to show list bullets in tooltip.
Nov 14, 2024
ee74650
Merge latest updates from main.
Nov 14, 2024
81ab282
Adjust how examples tooltip content is generated.
Nov 15, 2024
2b1c4dd
Adjust styling to show list bullets (and fix margins).
Nov 15, 2024
1b60da4
Adjust styling to override styling that is too broad.
Nov 15, 2024
efc1e6a
Merge branch 'main' into LEMS-2457-show-format-options-as-list
Nov 15, 2024
886e67a
Merge branch 'main' into LEMS-2457-show-format-options-as-list
Nov 16, 2024
860e56b
Only show tooltip content as a list if two or more examples are provi…
Nov 18, 2024
7a6b4e7
Add unit test to guard against accidental tooltip content changes.
Nov 18, 2024
5ae985c
Merge branch 'main' into LEMS-2457-show-format-options-as-list
Nov 18, 2024
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
5 changes: 5 additions & 0 deletions .changeset/many-keys-smash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[Numeric Input] - Show format options as a list
18 changes: 13 additions & 5 deletions packages/perseus/src/__tests__/__snapshots__/server-item-renderer.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,21 @@ exports[`server item renderer should snapshot: initial render 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
Copy link
Member

Choose a reason for hiding this comment

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

What is this data-perseus-paragraph-index attribute? Is it like our version of tabIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question has been posted to the team.

>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down
15 changes: 12 additions & 3 deletions packages/perseus/src/components/input-with-examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,18 @@ class InputWithExamples extends React.Component<Props, State> {
render(): React.ReactNode {
const input = this._renderInput();

const examplesContent = _.map(this.props.examples, (example) => {
return "- " + example;
}).join("\n");
const examplesContent =
this.props.examples.length <= 2
? this.props.examples.join(" ") // A single item (with or without leading text) is not a "list"
: this.props.examples // 2 or more items should display as a list
.map((example, index) => {
// If the first example is bold, then it is most likely a heading/leading text.
// So, it shouldn't be part of the list.
return index === 0 && example.startsWith("**")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like both remaining usages of <InputWithExamples /> provides an examples list where the first item is indeed a bold heading (here and here).

Perhaps we can simplify this once input-number is truly gone because at that point we'll know exactly what is always passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the plan. We have follow-up tickets to refactor aspects of Numeric Input. Seeing as how InputWithExamples isn't used anywhere else, we are considering bringing this component into Numeric Input and simplifying the two code bases.

? `${example}\n`
: `- ${example}`;
})
.join("\n");

const showExamples =
this.props.shouldShowExamples && this.state.showExamples;
Expand Down
18 changes: 13 additions & 5 deletions packages/perseus/src/multi-items/__tests__/__snapshots__/multi-renderer.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,13 +1016,21 @@ exports[`multi-item renderer should snapshot: initial render 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down
22 changes: 9 additions & 13 deletions packages/perseus/src/styles/perseus-renderer.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,24 +541,20 @@
}
}

// TODO(joel) - remove?
.perseus-tooltip {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This style wasn't really used, other than to be a base later on in .perseus-formats-tooltip. So, I just moved this styling to where it is used.

.perseus-formats-tooltip {
background: #fff;
color: #777;
padding: 5px 10px;
width: 240px;
}

// CSS is evil; let's overwrite some styles. T_T
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No! CSS is not evil. Misunderstood, maybe. But definitely not evil.

.perseus-formats-tooltip {
.perseus-tooltip;
color: #777;
}
.framework-perseus .perseus-formats-tooltip .paragraph > ul {
padding: 0;
margin: -20px 0 -16px 0;
> li {
list-style-type: none;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no evidence that this was ever needed. A list should (almost) always look like a list.

/*
Extra specificity needed to override other styles that are too broad.
Once we get a better framework in place (like CSS Modules), we can fix this selector.
*/
.framework-perseus .perseus-formats-tooltip .paragraph,
.framework-perseus .tooltipContainer .perseus-formats-tooltip .paragraph ul {
margin: 0;
}

.box-shadow(@shadow: 0 1px 3px rgba(0,0,0,0.25)) {
Expand Down
54 changes: 39 additions & 15 deletions ...ges/perseus/src/widgets/graded-group-set/__snapshots__/graded-group-set-jipt.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,21 @@ exports[`graded-group-set should render all graded groups 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down Expand Up @@ -330,13 +338,21 @@ exports[`graded-group-set should render all graded groups 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down Expand Up @@ -557,13 +573,21 @@ exports[`graded-group-set should render all graded groups 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down
18 changes: 13 additions & 5 deletions packages/perseus/src/widgets/graded-group-set/__snapshots__/graded-group-set.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,21 @@ exports[`graded group widget should snapshot 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down
36 changes: 26 additions & 10 deletions packages/perseus/src/widgets/group/__snapshots__/group.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,13 +796,21 @@ exports[`group widget should snapshot: initial render 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down Expand Up @@ -1001,13 +1009,21 @@ exports[`group widget should snapshot: initial render 1`] = `
<div
class="paragraph"
data-perseus-paragraph-index="0"
>
<div
class="paragraph"
>
<strong>
Your answer should be
</strong>

</div>
</div>
<div
class="paragraph"
data-perseus-paragraph-index="1"
>
<ul>
<li>
<strong>
Your answer should be
</strong>
</li>
<li>
an integer, like
<span
Expand Down
Loading