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

fix(app): specify component for markdown uls #14997

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

sfoster1
Copy link
Member

When these are undefined, if react-markdown tries to render them it will throw and that will crash the app. With these implemented with the same StyledText passthrough as other text elements, it won't crash anymore - though our styling rules aren't applied to the actual text in the ul.

Closes RQA-2566
Closes RQA-2587

When these are undefined, if react-markdown tries to render them it will
throw and that will crash the app. With these implemented with the same
StyledText passthrough as other text elements, it won't crash anymore -
though our styling rules aren't applied to the actual text in the ul.
@sfoster1 sfoster1 requested a review from mjhuff April 24, 2024 17:04
@sfoster1 sfoster1 requested a review from a team as a code owner April 24, 2024 17:04
@mjhuff
Copy link
Contributor

mjhuff commented Apr 24, 2024

I've thrown this branch on CI to test it. For some reason, locally building isn't sufficient for reproing the initial error, and the modal renders locally without issue despite being undefined.

https://github.com/Opentrons/opentrons/actions/runs/8820595504

Will leave a review after that's done.

@sfoster1
Copy link
Member Author

I've thrown this branch on CI to test it. For some reason, locally building isn't sufficient for reproing the initial error, and the modal renders locally without issue despite being undefined.

https://github.com/Opentrons/opentrons/actions/runs/8820595504

Will leave a review after that's done.

It reprod on dev mode for me, but you have to be actually trying to render the elements that are used here, at least for me. So there have to be uls in the release notes.

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

Ah that makes sense, thanks for explaining.

This looks good. We probably will need to follow up on this to get it matching designs (or get Ed to change the way we do the .md), but this is definitely better than the whitescreening! Thanks for fixing!

@sfoster1 sfoster1 merged commit 0493c5c into edge Apr 24, 2024
46 checks passed
@sfoster1 sfoster1 deleted the RQA-2566-fix-markdown-render branch April 24, 2024 18:52
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
When these are undefined, if react-markdown tries to render them it will
throw and that will crash the app. With these implemented with the same
StyledText passthrough as other text elements, it won't crash anymore -
though our styling rules aren't applied to the actual text in the ul.

Closes RQA-2566
Closes RQA-2587
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.

None yet

2 participants