Add support for radios printing #12108
Conversation
calixteman
force-pushed
the
radio
branch
2 times, most recently
from
8f19bef
to
8e25e04
Compare
lundjordan
added
the
Gecko 81
|
When the checkbox PR is merged, I think this only requires a rebase and a unit test similar to the checkbox one but for radio buttons. We should also move the existing checkbox printing unit test a bit up to group the checkbox tests apart from the radio button tests. |
|
I added a test so this PR is ready to be reviewed. |
src/core/annotation.js
Outdated
| @@ -1088,6 +1088,8 @@ class ButtonWidgetAnnotation extends WidgetAnnotation { | |||
| break; | |||
| } | |||
| } | |||
| this.checkedAppearance = normalAppearanceState.get(this.data.buttonValue); | |||
There was a problem hiding this comment.
Let's add a newline above this line like we did for the checkboxes to separate the appearance from the button value.
src/core/annotation.js
Outdated
| @@ -1088,6 +1088,8 @@ class ButtonWidgetAnnotation extends WidgetAnnotation { | |||
| break; | |||
| } | |||
| } | |||
| this.checkedAppearance = normalAppearanceState.get(this.data.buttonValue); | |||
| this.uncheckedAppearance = normalAppearanceState.get("Off") || null; | |||
There was a problem hiding this comment.
Hm, this was already there, but it might be better to rename normalAppearanceState to normalAppearance here too like we did for the checkboxes to match the specification more closely.
test/unit/annotation_spec.js
Outdated
| @@ -1741,6 +1741,93 @@ describe("annotation", function () { | |||
| done(); | |||
| }, done.fail); | |||
| }); | |||
|
|
|||
| it("should render radios for printing", function (done) { | |||
There was a problem hiding this comment.
radios -> radio buttons for consistency with the other tests.
| ); | ||
| return annotation; | ||
| }, done.fail) | ||
| .then(annotation => { |
There was a problem hiding this comment.
Testing the unchecked flow is a pretty good addition that we didn't think of when the checkboxes were made. Could we perhaps add a second commit to the PR that does two things:
- Move the checkbox unit test from line 1659 to to line 1624 so it's grouped with the other checkbox tests (now it's a checkbox test in between radio buttons tests which makes it a bit hard to find).
- Introduce the same flow for testing the unchecked case there too.
Doing so would be a nice way to improve checkbox test coverage and for consistency.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/72a18cd80f38fa0/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/72a18cd80f38fa0/output.txt Total script time: 3.48 mins Published |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/9b6cfd509218d91/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/b4c46fb92241266/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/9b6cfd509218d91/output.txt Total script time: 26.79 mins
Image differences available at: http://54.67.70.0:8877/9b6cfd509218d91/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/b4c46fb92241266/output.txt Total script time: 29.96 mins
Image differences available at: http://54.215.176.217:8877/b4c46fb92241266/reftest-analyzer.html#web=eq.log |
|
Another piece of the puzzle done. Thank you! |
No description provided.