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

Feature - Line highlighting within code block #2469 #2682

Merged
merged 14 commits into from
Dec 24, 2018

Conversation

duartefrazao
Copy link
Contributor

@duartefrazao duartefrazao commented Dec 5, 2018

Description

This PR solves the #2469 issue, it allows the user to highlight lines of code within a snippet of code by clicking on the line number. This highlight is not temporary, it persists when exiting Boostnote. Used CodeMirror built-in functionality to do this.

To use this feature you just need to click the line number of the respective line you want to highlight.

Before

screenshot from 2018-12-05 13-19-51

After

screenshot from 2018-12-05 13-20-18

Issue fixed

Issue #2469 Line highlighting within code block

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 Dec 6, 2018
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Can you provide some info on how to use this feature?

browser/components/CodeEditor.js Outdated Show resolved Hide resolved
Now iterates over highlighted lines instead of all lines of the snippet
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 6, 2018

Hmm...That's strange, I can't highlight the line by clicking on the gutter. Can you check it?

This makes the default snippet also handle highlight on the lines, because this snippet is created in the code without the normal snippet constructor
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 8, 2018

Please fix your eslint error

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 8, 2018

Also I discover that when my snippet doesn't contain linesHighlighted then the feature won't work at all. You should fallback linesHighlighted to [] if it not existed. Also, can you apply this feature to markdown note too?

Now iterates in the SnippetNoteDetail constructor the snippets and if linesHighlighted is not defined assigns an empty array
@duartefrazao
Copy link
Contributor Author

Fixed the issues you pointed.
Will replicate the feature to markdown note.

@ZeroX-DG
Copy link
Member

I can still reproduce the old bug. You should fallback linesHighlighted to [] if it is null instead of stopping the whole process.

@duartefrazao
Copy link
Contributor Author

Could you please tell me how to reproduce that bug?
Is it a default note, an old note pre pull request or a note that you changed?

@ZeroX-DG
Copy link
Member

It's an old default snippet note generated by Boostnote.

@duartefrazao
Copy link
Contributor Author

I've been trying to replicate this old bug but with no sucess.
I've tried to delete everyting Boostone related and download a past version and then open with my version of the pull request and it still works...
Could you detail the steps you took as well as operating system you're using?

@duartefrazao
Copy link
Contributor Author

I've tested in several computers and I can't replicate the bug anymore, please give me detailed instructions to replicate your bug.

@duartefrazao
Copy link
Contributor Author

I've also added this functionality to markdown notes.

@ZeroX-DG
Copy link
Member

I have to manually added linesHighlighted to the first note and leave the second note alone (the default note of Boostnote).
bbb

Fixed a bug where a note or snippet is created before the pull request and you ran Boostnote for the first time after the pr and you firstly created a note or markdown and only then returned to the old default notes and you couldn't highlight
@duartefrazao
Copy link
Contributor Author

I've made the necessary corrections to fix the bug.
By adding the functionality you asked a similar bug also appeared with the Markdown notes too, which is also fixed.
To easily test this bugs and ask others for help testing I made a simple script that makes it easy to replicate this situation. If you want to take a look or use it's here

@duartefrazao
Copy link
Contributor Author

I've been testing more the pr and I found another bug that I will try to solve by tomorrow.

Lines now save correctly with different inputs, making sure that different inputs like enter, delete, paste and where it's deleted stay consistent when saving.
Included in the create/update  snippet/note tests the structure from lines highlighting saved to the files.
@duartefrazao
Copy link
Contributor Author

This new commit contains two main parts:

  • Included the data structure holding past values of the lines highlighted for each note/snippet saved in the create and update snippet/note tests
  • Made the feature robust to the following variables when editing the note/snippet:
    • Type of action (delete, input, paste, undo and insert)
    • How many lines affected (e.g. be it a single line delete or multi-line)
    • Where the action starts and ends (e.g. if you end a delete on the middle of a line the highlight should be kept but if you end on the begging of the line the highlight should be removed)

@duartefrazao
Copy link
Contributor Author

@ZeroX-DG I'm also part of the same class that has recently contributed to Boostnote. As some of them mentioned It would really help my group's grade if we knew if the pull request is going to be accepted. Thank you very much!

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Everything works fine now, please change your code before I can approve the PR.

browser/components/MarkdownEditor.js Outdated Show resolved Hide resolved
browser/components/MarkdownSplitEditor.js Outdated Show resolved Hide resolved
@duartefrazao
Copy link
Contributor Author

duartefrazao commented Dec 22, 2018

Made the changes you asked, @ZeroX-DG . Thank you for explaining πŸ˜ƒ

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM πŸŽ‰ Thank you for your contribution and good luck with your course!

@ZeroX-DG ZeroX-DG added approved πŸ‘ Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 22, 2018
@duartefrazao
Copy link
Contributor Author

Thank you very much, and thank you for guiding us on submitting our pull request! Hope our work benefits Boostnote πŸ˜ƒ

@Rokt33r Rokt33r added next release (v0.11.13) and removed approved πŸ‘ Pull request has been approved by sufficient reviewers. labels Dec 24, 2018
@Rokt33r Rokt33r merged commit 2df0f1b into BoostIO:master Dec 24, 2018
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.

None yet

3 participants