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.

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

Set tab order when document is tagged #1449

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

acrollet
Copy link

Changes

fixes #1260

relates to department-of-veterans-affairs/va.gov-team#58587

This PR sets tab order to "Structure" when a document is tagged.

  • Unit Tests
  • Documentation - N/A
  • Update CHANGELOG.md
  • Ready to be merged

Copy link
Contributor

@insightfuls insightfuls left a comment

Choose a reason for hiding this comment

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

@acrollet , firstly, let me say thanks for putting this together. Despite being simple, I hadn't managed to look at it, and having something concrete to look at is really helpful. This is certainly put together well, and it's good to see that it's tested. For a couple of reasons I'll explain inline, I think a slightly different approach would be preferable, though. I think it should be easy to adjust, if you can spare a little more time for this.

lib/page.js Outdated Show resolved Hide resolved
Copy link
Contributor

@insightfuls insightfuls left a comment

Choose a reason for hiding this comment

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

Thanks @acrollet , overall, this looks great. There's nothing wrong with it, so I'm happy to give an approving review. I've left a comment inline, however, suggesting simplifying a condition, which also makes it more consistent with nearby code.

lib/mixins/markings.js Outdated Show resolved Hide resolved
lib/page.js Show resolved Hide resolved
Copy link
Author

@acrollet acrollet left a comment

Choose a reason for hiding this comment

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

Adding suggestion

lib/mixins/markings.js Outdated Show resolved Hide resolved
@insightfuls
Copy link
Contributor

@blikblum , I'm happy for this to be merged if you are, or would you like @liborm85 or @devongovett to give it a review as well?

@acrollet
Copy link
Author

acrollet commented May 26, 2023

acrollet@MacBook-Air pdfkit % npm run test                            

> pdfkit@0.13.0 test
> jest -i

 PASS  tests/visual/pdfmake/qrcode.spec.js
 PASS  tests/visual/pdfmake/tables.spec.js
 PASS  tests/visual/pdfmake/columns_simple.spec.js
 PASS  tests/visual/pdfmake/lists.spec.js
 PASS  tests/visual/pdfmake/page_references.spec.js
 PASS  tests/visual/pdfmake/toc.spec.js
 PASS  tests/visual/pdfmake/background.spec.js
 PASS  tests/visual/pdfmake/text_decorations.spec.js
 PASS  tests/visual/pdfmake/watermark.spec.js
 PASS  tests/visual/pdfmake/absolute.spec.js
 PASS  tests/visual/pdfmake/images.spec.js
 PASS  tests/unit/png.spec.js
 PASS  tests/unit/acroform.spec.js
 PASS  tests/visual/pdfmake/basics.spec.js
 PASS  tests/visual/vector.spec.js
 PASS  tests/unit/vector.spec.js
 PASS  tests/unit/annotations.spec.js
 PASS  tests/unit/attachments.spec.js
 PASS  tests/unit/text.spec.js
 PASS  tests/unit/virtual-fs.spec.js
 PASS  tests/unit/pdfa3.spec.js
 PASS  tests/unit/pdfa2.spec.js
 PASS  tests/unit/pdfa1.spec.js
 PASS  tests/unit/pattern.spec.js
 PASS  tests/unit/saslprep.spec.js
 PASS  tests/unit/trailer.spec.js
 PASS  tests/unit/reference.spec.js
 PASS  tests/unit/document.spec.js
 PASS  tests/visual/fonts.spec.js
 PASS  tests/unit/font.spec.js
 PASS  tests/unit/gradient.spec.js
 PASS  tests/visual/text.spec.js
 PASS  tests/unit/color.spec.js
 PASS  tests/unit/metadata.spec.js
 PASS  tests/unit/object.spec.js
 PASS  tests/unit/markings.spec.js

Test Suites: 36 passed, 36 total
Tests:       2 skipped, 142 passed, 144 total
Snapshots:   52 passed, 52 total
Time:        40.663 s
Ran all test suites.

@acrollet
Copy link
Author

Hi folks, just wondering if there's anything else needed on this PR. Thanks! 🙂

@acrollet
Copy link
Author

@blikblum @insightfuls just wondering if there's anything else needed or if this could be merged? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDF accessibility check
2 participants