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

DOCK-2428: Display markdown tables correctly #1827

Merged
merged 5 commits into from
Aug 11, 2023
Merged

DOCK-2428: Display markdown tables correctly #1827

merged 5 commits into from
Aug 11, 2023

Conversation

y-ng
Copy link
Collaborator

@y-ng y-ng commented Aug 1, 2023

Description
Some markdown tables do not render properly due to the presence of tab characters in the table. This PR removes tabs from table headers before parsing so that the html is generated correctly for the table.

Before:
Screenshot from 2023-08-01 16-48-48

After:
Screenshot from 2023-08-01 16-47-06

Review Instructions
Go to the info tab for a workflow with a table in the description (e.g., the workflow mentioned in the related ticket /workflows/github.com/aofarrel/tree_nine/tree_nine:main?tab=info) and make sure the table is displayed properly.

Issue
DOCK-2428

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@y-ng y-ng self-assigned this Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (4de7eb3) 40.67% compared to head (3efff16) 40.68%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1827      +/-   ##
===========================================
+ Coverage    40.67%   40.68%   +0.01%     
===========================================
  Files          364      364              
  Lines        11249    11251       +2     
  Branches      2866     2866              
===========================================
+ Hits          4576     4578       +2     
  Misses        4377     4377              
  Partials      2296     2296              
Files Changed Coverage Δ
...hared/markdown-wrapper/markdown-wrapper.service.ts 63.15% <100.00%> (+4.33%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@y-ng y-ng requested review from a team, david4096, hyunnaye and svonworl and removed request for a team August 1, 2023 21:11
@denis-yuen
Copy link
Member

warning The version of Java (11.0.14.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

WIll create ticket

@svonworl
Copy link
Contributor

svonworl commented Aug 9, 2023

Would it work to replace each tab with four spaces, but just in the lines that start with |?
I'm a little bit concerned about the SonarCloud "Security Hotspots", it's tough to tailor complicated regexps so that they're not DOS-able.

@y-ng
Copy link
Collaborator Author

y-ng commented Aug 10, 2023

Would it work to replace each tab with four spaces, but just in the lines that start with |? I'm a little bit concerned about the SonarCloud "Security Hotspots", it's tough to tailor complicated regexps so that they're not DOS-able.

Yeah, the tables would still render correctly since the tabs are removed (and this gets rid of the security hotspots which is good). The only thing is that tabs in other lines beginning with '|" would also be replaced with spaces, but that might not be too big of a concern if it's not very common.

@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.14.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@y-ng y-ng merged commit bc64c68 into develop Aug 11, 2023
19 checks passed
@y-ng y-ng deleted the DOCK-2428 branch August 11, 2023 14:16
@y-ng y-ng mentioned this pull request Aug 23, 2023
11 tasks
@y-ng y-ng mentioned this pull request Aug 24, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants