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

Fixes rendering for checkboxes #1155

Conversation

Eclipse-Dominator
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator commented Mar 7, 2023

Summary:

Fixes #1152

Changes Made:

  • uses emoji to render checkboxes instead of using tag to bypass the html sanitizer.

Proposed Commit Message:

Checkboxes are not longer being rendered due to the sanitizer sanitizing html input tags

However, removing the sanitizer could be unsafe

Let's
* Fixes checkboxes being sanitized when rendered in markdown
* Replace checkbox rendering via the use of emojis 
* Addition of fontawesome icon library to standardize emoji font face

Copy link
Contributor

@cheehongw cheehongw left a comment

Choose a reason for hiding this comment

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

☑️ LGTM ☑️

Would be good to see how the emoji looks like on different OSes

@Eclipse-Dominator
Copy link
Contributor Author

Eclipse-Dominator commented Mar 8, 2023

As this is a replacement workaround, I do think it is a good idea to test if the emoji display functions normally on other systems and browsers.

Below is how it looked like on windows 11 edge and chrome browser
image

I'm not sure if this looks acceptable when displayed on other system if someone is free do try to test it out by pasting
◻️test1
☑️test2
in the markdown editor

@damithc
Copy link
Contributor

damithc commented Mar 8, 2023

Are we using an icon font such as fontawesome? In that case we can try using matching icons provided by the font, as they will look the same in all browsers.

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (d6eb065) 53.82% compared to head (e44e7c1) 53.81%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1155      +/-   ##
==========================================
- Coverage   53.82%   53.81%   -0.01%     
==========================================
  Files          99       99              
  Lines        2696     2698       +2     
  Branches      497      498       +1     
==========================================
+ Hits         1451     1452       +1     
- Misses        915      916       +1     
  Partials      330      330              
Impacted Files Coverage Δ
src/app/shared/lib/marked.ts 87.50% <100.00%> (-12.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@chunweii chunweii left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@kkangs0226 kkangs0226 left a comment

Choose a reason for hiding this comment

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

Checked that the issue has been fixed, LGTM.
Let's include the addition of font-awesome icons in the commit message as well.
Also, please follow this guide for crafting your commit messages. https://se-education.org/guides/conventions/git.html

Copy link
Contributor

@kkangs0226 kkangs0226 left a comment

Choose a reason for hiding this comment

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

LGTM

@kkangs0226 kkangs0226 merged commit 0a5d90a into CATcher-org:master Mar 20, 2023
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.

Editing bug descriptions: checkbox items are previewed as bullet points
6 participants