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.

Sign up for GitHub

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

Jump to bottom

Fix regular expression related checkbox logic #3001

Merged
merged 3 commits into from
May 29, 2019
Merged

Fix regular expression related checkbox logic #3001

merged 3 commits into from
May 29, 2019

Conversation

mpls104
Copy link
Contributor

@mpls104 mpls104 commented May 10, 2019 •
edited
Loading

evidence

Description

As described in issue, checkbox was unclickable in quotation.
Fixed regular expression on checkbox logic and added some test patterns.

This is my first contribution to OSS in my life. 😄
Please kindly point out if there is something wrong 🙏

Issue fixed

#2829

Type of changes

  • 🔘 Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • 🔘 I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label May 11, 2019
@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 11, 2019 •
edited
Loading

I'm gonna be criticizing as hard as I can 🤣 Anyway, welcome to OSS

@ZeroX-DG
Copy link
Member

It currently doesn't work for a nested quote like this. I don't know if we should support it.
image

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 11, 2019
@mpls104
Copy link
Contributor Author

mpls104 commented May 13, 2019

Thank you for checking precisely 😄
I think it's better to support this and done some changes.
What do you think @ZeroX-DG ?

@ZeroX-DG
Copy link
Member

Well, I'll need confirmation from the "higher" power @Rokt33r

@AWolf81
Copy link
Contributor

AWolf81 commented May 28, 2019

LGTM.

Just one thing I've noticed is that sub-elements are striked through if the top element is checked. I think you haven't introduced the issue but it's a bit weird and it would be probably better to not have them striked as they're not handled in the action item done count. See screenshot below, everything seems done but the counter says 60%.
But I think we could improve that in a separate PR if it's difficult to fix.

grafik

I'll have a look at your regex updates in a minute and leave an inline comment if there is something to improve.

AWolf81
AWolf81 reviewed May 28, 2019
View reviewed changes
browser/components/MarkdownEditor.js
const checkReplace = /\[x\]/i
const uncheckReplace = /\[ \]/
const checkedMatch = /^(\s*>?)*\s*[+\-*] \[x]/i
const uncheckedMatch = /^(\s*>?)*\s*[+\-*] \[ ]/
Copy link
Contributor

Choose a reason for hiding this comment

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

I've first thought there are missing escape sequences in the regex but I think it's OK to omit them.
I did a quick test in the following regex101 - just wondering why the nested checkboxes are not matching there.
Is it because in Boostnote it is running in a loop or do I have a mistake in the regex101 test patterns?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a line pattern and only matched the first occurrence

@ZeroX-DG
Copy link
Member

@AWolf81 The nested checkbox is strikethrough is because of the CSS property

text-decoration: line-through;

If that CSS property is applied to the parent element, the children element will be affected too which is a bit weird. You're right, it should be solved in a separated issue, it may be difficult to solve.

@arcturus140
Copy link
Contributor

i think this strike through behaviour is intended, but I also think it is weird 😣

@Rokt33r Rokt33r self-requested a review May 29, 2019 13:51
@Rokt33r
Copy link
Member

Rokt33r commented May 29, 2019

LGTM. And I'm also thinking it is a bit weird. Let's fix it in other pr.

@Rokt33r Rokt33r merged commit 244a28c into BoostIO:master May 29, 2019
@Rokt33r Rokt33r removed the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label May 29, 2019
@Rokt33r Rokt33r added this to the v0.12.0 milestone May 29, 2019
@Rokt33r Rokt33r mentioned this pull request Jun 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arcturus140 arcturus140 arcturus140 left review comments

@AWolf81 AWolf81 AWolf81 left review comments

@Rokt33r Rokt33r Awaiting requested review from Rokt33r

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mpls104 @ZeroX-DG @AWolf81 @arcturus140 @Rokt33r