-
Notifications
You must be signed in to change notification settings - Fork 126
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
Strip tags on clearThought #801
Strip tags on clearThought #801
Conversation
@raineorshine We cannot render html within placeholder so it would make sense to just strip away all the tags. What do you think ? |
Yes, that's correct. It won't have any non-formatting tags anyway since they have already been stripped out during editing. |
src/shortcuts/clearThought.js
Outdated
@@ -4,6 +4,8 @@ import { | |||
setSelection, | |||
} from '../util' | |||
|
|||
const tagsRegex = /<(?:.|\n)*?>/gmi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have two other regex definitions that strip tags/html. Could you assess whether they can be combined into a single regex, and if so, have them all (including clearThought
) reference that regex from constants
? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raineorshine The REGEXP_HTML
in constants /<\/?[a-z][\s\S]*>/i
doesn't do strict tag checks. <p>test>
as a whole will pass the regex test. Is this intentional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REGEXP_HTML
is currently used to detect HTML, not replace tags. I'm not sure what you mean by "strict tag checks."
You should be able to see exactly where each of the HTML-related regular expressions are being used, and determine if any of them can be combined based on their use.
@shresthabijay Any updates on this? |
fd377f6
to
5c5802f
Compare
@raineorshine The |
Perfect, thanks! |
fixes #743