Skip to content

LG-7620: Update approach to IPP CTA newline control#7069

Merged
allthesignals merged 3 commits intomainfrom
wmg/7620-ipp-cta-alt
Oct 4, 2022
Merged

LG-7620: Update approach to IPP CTA newline control#7069
allthesignals merged 3 commits intomainfrom
wmg/7620-ipp-cta-alt

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Oct 3, 2022

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Inserts non-breaking spaces where appropriate, leveraging formatHtml's custom element handlers feature.

Fixes an edge case from #7058 while avoiding the (over) complexity of this approach.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Manually visual feedback looks correct

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:

image

After: Screen Shot 2022-10-02 at 8 18 13 PM

Oct-02-2022 20-32-08

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could just insert the non-breaking space directly?

Suggested change
are_you_near: 'Are you located near Washington,<nbsp/>D.C. <wbr/>or<nbsp/>Baltimore,<nbsp/>MD?'
are_you_near: 'Are you located near Washington, D.C. or Baltimore, MD?'

It seems to get past the YAML normalization without issue. I think it does pose a risk to be confusing to developers since visually it appears the same as a normal space, though at least in VSCode there is some additional highlighting on the irregular whitespace:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, duh! For some reason I thought there was an issue with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that we ought to export the character for reuse, since it seems rather incidental for implementation and not something that one would expect as a named export of an encrypted upload module. I'd prefer if either (a) we duplicated it in DocumentCaptureTroubleshootingOptions or (b) moved it to some place where one could reasonably expect a named export of the sort (e.g. packages/document-capture/services/typography.ts, packages/typography/index.ts).

@allthesignals allthesignals requested a review from aduth October 4, 2022 15:45
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

troubleshooting:
headings:
are_you_near: 'Are you located near Washington, D.C. <br/>or Baltimore, MD?'
are_you_near: 'Are you located near Washington, D.C. <wbr/>or Baltimore, MD?'
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it might be worth adding a code comment indicating that there are intentional irregular, non-breaking whitespace in this sentence?

@allthesignals allthesignals merged commit aea1af6 into main Oct 4, 2022
@allthesignals allthesignals deleted the wmg/7620-ipp-cta-alt branch October 4, 2022 17:47
jskinne3 pushed a commit that referenced this pull request Oct 12, 2022
* changelog: Improvements, In-Person Proofing, Adjust approach to newline control CTA

* Add minor comment
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.

2 participants