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(card): ensure copy can render markdown #7871

Merged
merged 12 commits into from
Jan 5, 2022

Conversation

IgnacioBecerra
Copy link
Contributor

@IgnacioBecerra IgnacioBecerra commented Dec 13, 2021

Related Ticket(s)

#7481

Description

This PR ensures that the copy elements in Card and Content Section can render markdown properly. The approach used in Card queries the slotted p tag and applies the markdown utility function. While a simpler method may be creating a card-copy component that extends DDSMarkdown, I refrained from doing so to avoid a breaking change for adopters using Card. Feel free to let me know what you think about doing that approach instead and simply mentioning it in the release notes.

Note:
Testing markdown in Storybook seems to only work once (i.e. inputting *test*, deleting it, and trying the same thing will result in the text being wrapped in asterisks without formatting.

However, if an adopter uses markdown in the copy of the Card component, it will render properly.

Screen Shot 2021-12-13 at 3 10 55 PM

Changelog

Changed

  • Card copy can now use markdown
  • Content section copy now extends DDSMarkdown component

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7871/index.html

Built with commit: e1680836dc7391e62ac1f23e5702b9eeda5b78b8

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Dec 14, 2021

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7871/index.html

Built with commit: e1680836dc7391e62ac1f23e5702b9eeda5b78b8

emyarod
emyarod previously approved these changes Dec 14, 2021
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

creating a card-copy component that extends DDSMarkdown

I think this would be fine given that the component was supposed to have this functionality already. Since the concern is around it being a breaking change it should be ok to make this update in v2

looks good to me, just needs a few CI test updates

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

one thing I noticed, characters like # and & won't render properly in the card copy with the markdown parser

related #7469

@emyarod emyarod self-requested a review December 14, 2021 16:36
@emyarod emyarod dismissed their stale review December 14, 2021 16:36

should be comment

@IgnacioBecerra
Copy link
Contributor Author

IgnacioBecerra commented Dec 14, 2021

@emyarod I was able to get the # to render, but yeah the ampersand was getting removed. It seemed to have been getting stripped using the regex expression after the & became &amp when parsed through marked(), so I removed that part from the expression so it can remain there. Let me know if that part of the regex expression should actually remain.

Screen Shot 2021-12-14 at 2 20 14 PM

Screen Shot 2021-12-14 at 2 19 43 PM

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me! should we create a separate issue for the storybook rendering problems?

@jeffchew jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Dec 15, 2021
@jeffchew
Copy link
Member

@IgnacioBecerra just double checking if the font size change in the link list was intentional here? or perhaps this is coming from an outdated baseline?

image

@jeffchew jeffchew removed the Ready to merge Label for the pull requests that are ready to merge label Dec 15, 2021
@IgnacioBecerra
Copy link
Contributor Author

@jeffchew yeah that change isn't coming from this PR, the actual font size isn't changing with these changes

@jeffchew
Copy link
Member

@IgnacioBecerra there seems to be something going on with the card group too, I don't see this on latest canary and double checked that it's happening on your fork:

image

@IgnacioBecerra
Copy link
Contributor Author

IgnacioBecerra commented Dec 15, 2021

@jeffchew Found out what was happening -- once there is a dds-content-block-copy in the page, the renderer function it uses to convert its text to <dds-content-block-paragraph overrides the default one in the markdownToHtml utility, which affects the rest of the components using it.

In other words, any use of the utility after using content-block-copy will always be wrapped with at <dds-content-block-paragraph, which includes the p tags in Card that we’re modifying in this PR. A way to circumvent the big text in the cards is to just remove the styles from the rendered content-block-paragraph element.

As mentioned in an earlier comment, creating a dds-card-copy component that extends ddd-markdown will solve the problem properly as it will have its own renderer, but since it will be a breaking change we could have this feature for v2 next year.

@jeffchew
Copy link
Member

@IgnacioBecerra thank you for updating, the diffs look much better!

There's still a slight diff for this, wondering if we need a design eye to take a glance at this?

image

Copy link
Member

@jeffchew jeffchew left a comment

Choose a reason for hiding this comment

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

Thanks @IgnacioBecerra !

@jeffchew jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Jan 5, 2022
@kodiakhq kodiakhq bot merged commit 7b35d2d into carbon-design-system:main Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants