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: Render markdown correctly for short descriptions #6683

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

asmigarg04
Copy link

What type of PR is this? (check all applicable)

Fixes issue #6674

Describe this PR

This PR addresses a longstanding issue where markdown in the short descriptions of project cards (e.g., URLs) was not rendered correctly. This bug caused markdown syntax (like text) to display as plain text instead of being interpreted as clickable links.

##Changes made
Updated the ProjectCard component to ensure the shortDescription field renders markdown properly.
Integrated a markdown-to-HTML parser (react-markdown) for safe and efficient markdown rendering.

@spwoodcock
Copy link
Member

spwoodcock commented Jan 10, 2025

Thanks for the PR @asmigarg04 !

While this is a great issue to address, there are a couple of problems with the PR:

  • It uses the react-markdown, but that package is not installed in the package.json on the frontend.
  • The backend for TM has a dependency called Markdown installed already. So I think the approach used in this codebase is to render the markdown text on the backend, then pass it already rendered to the frontend.

One approach could be to add a markdown renderer to the frontend, as you have done in this PR (I will let the devs decide on if that is a good approach for them).

Or another would be to run the project description through the markdown renderer on the backend first, before it's passed to the frontend and displayed.

Thanks again! Let's hear back from the team to see what they think is best 😄

@asmigarg04
Copy link
Author

Thanks @spwoodcock for reviewing my PR. Please let me know if in any way I can help to improve and work on this issue further.

@asmigarg04
Copy link
Author

Hi ,@spwoodcock . Can you please let me know if there are any updates regarding this issue?

@spwoodcock
Copy link
Member

spwoodcock commented Jan 13, 2025

Hi @asmigarg04! Sorry it's down to the dev team to consider the correct approach to this and reply (my hunch is this needs to be done on the backend, returning a pre-rendered HTML response from the Markdown) - I'm assuming they might have other urgent priorities to attend to for now 😅 Thanks for the patience 🙏

@asmigarg04
Copy link
Author

It's alright. Thanks for informing.

@royallsilwallz royallsilwallz self-assigned this Jan 15, 2025
@royallsilwallz
Copy link
Contributor

Hi @asmigarg04, thank you for addressing the issue and raising a PR. 🚀

I have reviewed your PR and have the following comments -

  • There is already a util function (htmlFromMarkdown) that handles the html render so react-markdown package is not required.
  • You can see the implementation example in this line of code.
  • I see there are empty files that are not relevant to the PR (babel-jest, chokidar, etc). Can you remove those?
  • Also, can you squash all the commits for these into one or two only?

Feel free to ask if you have any further queries. 🙏

Also, thank you @spwoodcock for the quick response, review and feedback! ✨

@royallsilwallz royallsilwallz self-requested a review January 28, 2025 04:40
@royallsilwallz royallsilwallz removed their assignment Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants