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

Update alert rendering to fix visual bug #3680

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

zachmargolis
Copy link
Contributor

Changes proposed in this pull request:

There was a bug in the rendering of https://handbook.tts.gsa.gov/travel-and-leave/travel-guide-table-of-contents/

I spot-checked one more alertbox and it seems to be ok with link content still

before after
Screenshot 2023-08-07 at 1 53 52 PM Screenshot 2023-08-07 at 1 53 56 PM
Screenshot 2023-08-07 at 1 54 07 PM Screenshot 2023-08-07 at 1 54 07 PM

Bonus Changes

Also updates README instructions for local development

security considerations

  • Removing an extra layer of HTML processing should be safe?

@zachmargolis zachmargolis requested a review from a team as a code owner August 7, 2023 20:57
@mgwalker
Copy link
Member

mgwalker commented Aug 7, 2023

The alert for the software warning one is broken now, but that's not surprising. That one and the travel guide make incompatible assumptions about the alert.

I'll argue that the software tools usage is correct and the render filter should be applied. The problem with the travel guide alert is that it is also rendering the content, so it's basically being run through the liquid parser twice, but the second pass uses the markdown-it renderer, which by default converts HTML to plaintext. By changing that configuration to preserve HTML, the render filter works as expected!

This was a really good find!

@zachmargolis
Copy link
Contributor Author

Ah thanks for helping me dig one layer deeper!

@zachmargolis zachmargolis merged commit 7b17339 into main Aug 7, 2023
7 checks passed
@zachmargolis zachmargolis deleted the margolis-update-alert-rendering branch August 7, 2023 22:10
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