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 bold texts loosing formatting after converting to markdown and back to wysiwyg #15

Merged
merged 5 commits into from
Jul 21, 2020

Conversation

Shulammite-Aso
Copy link

Will be adding test for this as well.

@Shulammite-Aso
Copy link
Author

Shulammite-Aso commented Jul 16, 2020

@jywarren @sagarpreet-chadha @NitinBhasneria @shreyaa-sharmaa @keshav234156 please take a look.

@sagarpreet-chadha very happy to be having you around!!🎉

src/woofmark.js Outdated Show resolved Hide resolved
@Shulammite-Aso
Copy link
Author

#17 Should be merged before this, I think.

Copy link

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

awesome changes 🎉

As a general rule, we should avoid using var as anyone can change that variable unintentionally, we should try to use const as much as possible. Thanks 😄

src/woofmark.js Outdated
@@ -218,7 +218,21 @@ function woofmark (textarea, options) {
if (currentMode === 'html') {
textarea.value = parse('parseHTML', textarea.value).trim();
} else {
textarea.value = parse('parseHTML', editable).trim();
var regexp = /\*\*[A-Z][^*]+ \*\*/gi;

Choose a reason for hiding this comment

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

Can we have a better name for this regex? Instead of using regex and regex2 as variable name, can we have a separate utils file where each regex is defined (can be done in a separate PR)

Copy link
Author

Choose a reason for hiding this comment

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

was intending to comment the code, and will think of something more suitable to rename the variable with like your're suggesting. About a utils file, do you mean putting this code in a function and importing from a utils file?

Choose a reason for hiding this comment

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

Yes only this regex line but it can be done later. No worries 👍

src/woofmark.js Outdated
var reg = textarea.value.match(regexp);

for (let i = 0; i <= reg.length - 1; i++) {
var regexp2 = /\*\*[A-Z][^*]+ \*\*/i;

Choose a reason for hiding this comment

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

we are re declarating this regex on each for iteration.

Copy link
Author

Choose a reason for hiding this comment

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

regexp2 should go outside the iteration sorry. will fix this.

@Shulammite-Aso
Copy link
Author

Shulammite-Aso commented Jul 17, 2020

awesome changes

As a general rule, we should avoid using var as anyone can change that variable unintentionally, we should try to use const as much as possible. Thanks

Okay! @sagarpreet-chadha, only the codebase uses var every other place. Should i still use const anyway?

@sagarpreet-chadha
Copy link

Yes i believe we can use const 👍 Thanks :)

@cypherean
Copy link

Hey @Shulammite-Aso can you explain what the issue is?

@Shulammite-Aso
Copy link
Author

Hey @Shulammite-Aso can you explain what the issue is?

so @shreyaa-sharmaa sometimes when you make some texts bold in Rich mode and change to markdown
then back to rich, you get something like **text ** instead of the original text. In publiclab/post, the editor will warn you about using markdown while in Rich mode.
something similar has been reported at bevacqua#54 and here publiclab/plots2#7320

Noticed this usually happens when you have space between the last character and the closing bold tag, like this; **text ** instead of text. One can write texts like this in rich mode without knowing it, and woofmark has no way to remove or ignore the space, so that it can be properly read as a bold formatted text when parsing it to HTML.

@Shulammite-Aso
Copy link
Author

just recalling that this may also be happening with italic. Checked and confirmed it is. Will try to also fix this for italic texts

@Shulammite-Aso
Copy link
Author

@sagarpreet-chadha @jywarren please do check this out.

For the test, i made a different test.html file instead of using the existing index.html. This is so that we can simulate the typing in a clean editor area. The two are exactly the same, only difference is that there are no texts in the textarea.

@Shulammite-Aso
Copy link
Author

if we agree that everything is okay with this, i can now open the PR at bevacqua/woofmark.

Thanks!!!

@sagarpreet-chadha
Copy link

We can use the same index.html also to avoid redundancy, using id of textarea and using JS, we can remove text in the starting of it block of that test. Would you like to try that?

@jywarren
Copy link
Owner

jywarren commented Jul 20, 2020 via email

@Shulammite-Aso
Copy link
Author

We can use the same index.html also to avoid redundancy, using id of textarea and using JS, we can remove text in the starting of it block of that test. Would you like to try that?

have just removed the test html file @sagarpreet-chadha @jywarren and it works fine.

@jywarren jywarren merged commit e6f56ea into jywarren:plots2 Jul 21, 2020
@jywarren
Copy link
Owner

Nicely done - if you'd like to add some more assertions as mentioned above, let's do it in a follow-up! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants