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

Adds functionality to display tags under the open note. #893

Merged
merged 4 commits into from Nov 7, 2018

Conversation

Projects
None yet
3 participants
@Abijeet
Contributor

Abijeet commented Oct 16, 2018

Towards #469

Signed-off-by: Abijeet abijeetpatro@gmail.com

Adds functionality to display tags under the open note.
Towards #469

Signed-off-by: Abijeet <abijeetpatro@gmail.com>
@@ -184,7 +186,7 @@ class NoteTextComponent extends React.Component {
// Note:
// - What's called "cursor position" is expressed as { row: x, column: y } and is how Ace Editor get/set the cursor position
// - A "range" defines a selection with a start and end cusor position, expressed as { start: <CursorPos>, end: <CursorPos> }
// - A "range" defines a selection with a start and end cusor position, expressed as { start: <CursorPos>, end: <CursorPos> }

This comment has been minimized.

@Abijeet

Abijeet Oct 16, 2018

Contributor

Spaces at end of the line removed. Hope that's OK.

@@ -0,0 +1,48 @@
const React = require('react');

This comment has been minimized.

@Abijeet

Abijeet Oct 16, 2018

Contributor

Added TagList and TagItem components as we can do more with the tags under the note in the future.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Oct 24, 2018

Thanks @Abijeet, that looks pretty good already. The title says that it's WIP so does it mean it's not ready for testing/merging yet?

@Abijeet

This comment has been minimized.

Contributor

Abijeet commented Oct 25, 2018

@laurent22 - Yea, I'm running a few tests. I'll wind it up today.

Ensured tags in the dialog box and under the note appear in the same …
…order.

Few formatting tweaks.

Signed-off-by: Abijeet <abijeetpatro@gmail.com>
const tagItems = [];
if (tags || tags.length > 0) {
// Sort by id for now, but probably needs to be changed in the future.
tags.sort((a, b) => (a.id > b.id) ? 1 : ((b.id > a.id) ? -1 : 0));

This comment has been minimized.

@Abijeet

Abijeet Oct 25, 2018

Contributor

Sorting to ensure that the order of notes in the note-tag edit dialog box and under the note are same.

This should probably be changed to sort on the basis of name or something?

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Yes please let's sort them by name for now (ignoring case). If later on there's an option to sort tags in the sidebar, we'll apply it here too, but for now sorting them by name is fine.

This comment has been minimized.

@Abijeet

Abijeet Nov 6, 2018

Contributor

Hey, sorry missed this, will push another commit.

This comment has been minimized.

@Abijeet

Abijeet Nov 6, 2018

Contributor

Done, fixed.

Updated code to order tags by name in ascending order here and in the dialog box that appears when you modify tags for a note.

@Abijeet

This comment has been minimized.

Contributor

Abijeet commented Oct 25, 2018

@laurent22 - Ready for your review.

Screenshot

joplin-tags

Unit tests

Ran the following tests,

  • Adding a tag to a new note.
  • Updating an existing tag.
  • Deleting a tag removes it from the note.
  • Adding a new tag to a note.
  • Renaming a tag changes the tag in the note.
  • Tags appear in the same order on the dialog box and on the note.
  • Creating a new note.
@@ -426,6 +433,13 @@ class NoteTextComponent extends React.Component {
this.setState(newState);
if (!this.props.newNote) {

This comment has been minimized.

@Abijeet

Abijeet Oct 25, 2018

Contributor

Had to add this check. If not, was going into an infinite loop where state was getting updated repeatedly.

Since I'm updating the state, the componentWillReceiveProps was getting triggered again, where nextProps.newNote was still true, causing reloadNote to trigger again and again.

Not happy with this piece of code, but also not sure how to approach the problem.

This comment has been minimized.

@laurent22

laurent22 Nov 1, 2018

Owner

That component was not very well designed to begin with since I was a bit new to React when I wrote it. The proper solution probably is to refactor and not use componentWillReceiveProps, which is now deprecated, but I'll check and see if I can think of a simpler solution.

This comment has been minimized.

@laurent22

laurent22 Nov 7, 2018

Owner

For this, let's leave it as it is for now, and I'm going to add your comment above the line.

@Abijeet Abijeet changed the title from WIP - Adds functionality to display tags under the open note. to Adds functionality to display tags under the open note. Oct 25, 2018

@@ -434,6 +448,10 @@ class NoteTextComponent extends React.Component {
await this.reloadNote(nextProps);
} else if ('noteId' in nextProps && nextProps.noteId !== this.props.noteId) {
await this.reloadNote(nextProps);
} else if ('noteTags' in nextProps && this.areNoteTagsModified(nextProps.noteTags, this.state.noteTags)) {
await this.setState({

This comment has been minimized.

@laurent22

laurent22 Nov 1, 2018

Owner

Any reason why there's an "await" here? I don't believe setState returns a promise so the "await" won't do anything.

This comment has been minimized.

@Abijeet

Abijeet Nov 6, 2018

Contributor

Hmm...not sure why I added this. Removed this piece of code.

@@ -0,0 +1,23 @@
const React = require('react');

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

In TagItem.jsx and TagList.jsx, sometimes spaces are used for indentation, sometimes tabs. Please could you change them all to tabs?

This comment has been minimized.

@Abijeet

Abijeet Nov 6, 2018

Contributor

Hey, this is fixed. Since these are new files, VSCode did not have any prior indentation to detect, and used my defaults that is space.

Fixes issues raised during code review.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>
@Abijeet

This comment has been minimized.

Contributor

Abijeet commented Nov 6, 2018

@laurent22 - Modified as per code recommendations.

Refactored code to always display tags in ascending order.
This changes the order of the tags in the dialog box and below the tag title.

Signed-off-by: Abijeet <abijeetpatro@gmail.com>

@laurent22 laurent22 merged commit 18717ba into laurent22:master Nov 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 7, 2018

That looks good now, thanks a lot @Abijeet!

@laurent22 laurent22 referenced this pull request Nov 24, 2018

Closed

High cpu use #995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment