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

Desktop: Add toggling functionality for bold, italics and code in Editor Toolbar #2565

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

rabeehrz
Copy link
Contributor

@rabeehrz rabeehrz commented Feb 23, 2020

Fixes issue #2560

This PR adds a toggling function for bold, italics and code elements in the Editor Toolbar.

As mentioned in the issue:

You have a text bla, select it and click Bold. Then you get **bla**, you select **bla**, click Bold and get bla.

Improvements

Added italics toggling for wrapping using underscores

Italics

Before

Before

After

After

@rabeehrz
Copy link
Contributor Author

@PackElend label me please.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

I've noticed 1 issue and 1 room for improvement:

When you have a code block and select the code block (plus the bacticks) and use the code shortcut, the code block is rendered to normal text again (which was the idea), but an empty line is appended. So this has to be fixed. You have to make sure that there's no extra empty line.

As a suggestion, please also check for the underscore character for the emphasized text.
e.g. I write my emphasized text like _bla_, but when I select that and hit the I button, it turns into *_bla_*. see comment inline.

this.wrapSelectionWithStrings('*', '*', _('emphasized text'));
const selection = this.textOffsetSelection();
let string = this.state.note.body.substr(selection.start, selection.end - selection.start);
if (string.startsWith('*') && string.endsWith('*')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also check for underscore here.
if ((string.startsWith('*') && string.endsWith('*')) || (string.startsWith('_') && string.endsWith('_'))) {

Copy link
Contributor Author

@rabeehrz rabeehrz Feb 24, 2020

Choose a reason for hiding this comment

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

@tessus I've made the requested changes and updated the PR. Please review.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

Unfortunately the codeblock still leaves an extra empty line.

@tessus
Copy link
Collaborator

tessus commented Feb 24, 2020

There's one line between the code block and the text.

cb

Now there are 2 empty lines bwtween the code block and the text.

Copy link

@iyashwantsaini iyashwantsaini left a comment

Choose a reason for hiding this comment

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

use this in commandTextCode():

                     if (string.startsWith('```') && string.endsWith('```')) {
			this.wrapSelectionWithStrings('', '', '', string.substr(4, selection.end - selection.start - 8
		} else {
			this.wrapSelectionWithStrings(`\`\`\`${match[0]}`, `${match[0]}\`\`\``);
		}

This will fix the extra line appearing.

@rabeehrz rabeehrz force-pushed the issue-2560 branch 2 times, most recently from dcaf583 to f2e7b8c Compare February 26, 2020 03:05
@rabeehrz
Copy link
Contributor Author

use this in commandTextCode():

                     if (string.startsWith('```') && string.endsWith('```')) {
			this.wrapSelectionWithStrings('', '', '', string.substr(4, selection.end - selection.start - 8
		} else {
			this.wrapSelectionWithStrings(`\`\`\`${match[0]}`, `${match[0]}\`\`\``);
		}

This will fix the extra line appearing.

Thanks, @meyash
@tessus I've added the above change and that seemed to have solved the issue. Please review.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

LGTM. Tested.

@laurent22
Copy link
Owner

The four blocks of code you've added are all very similar. Could you refactor them into a single function please?

@rabeehrz
Copy link
Contributor Author

The four blocks of code you've added are all very similar. Could you refactor them into a single function please?

I've shortened the code a bit without making it very complicated.
I've used the old style for code block replacement because the two cases (single line and multiple line) should be handled separately.

Copy link
Contributor

@Rishabh-malhotraa Rishabh-malhotraa left a comment

Choose a reason for hiding this comment

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

Looks good to me, you can improve this code by adding regexp for searching but this is fine too

@tessus
Copy link
Collaborator

tessus commented Mar 6, 2020

Anything else that has to be done or can we merge?

@laurent22
Copy link
Owner

Looks good, thanks @rabeehrz!

@laurent22 laurent22 merged commit 67e5451 into laurent22:master Mar 6, 2020
@laurent22
Copy link
Owner

Did this feature ever worked? On the latest version if I select some text and click on bold it becomes **some text**, then if I click again it becomes ****some text****, so like it always did. Not sure if the feature got broken or if it had some bug. For now, I'm refactoring the text editor component and removing this feature as I don't know how to fix it.

@tessus
Copy link
Collaborator

tessus commented Apr 23, 2020

Yes, but you have to select the text first.

There's also a follow-up PR which I think does what you think it should do.

@tessus
Copy link
Collaborator

tessus commented Apr 23, 2020

#3077

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

Successfully merging this pull request may close these issues.

6 participants