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: Display selected tags under a note title #2217

Merged
merged 1 commit into from Jan 6, 2020
Merged

Desktop: Display selected tags under a note title #2217

merged 1 commit into from Jan 6, 2020

Conversation

Abijeet
Copy link
Contributor

@Abijeet Abijeet commented Dec 22, 2019

Follow up to #893

Now using middleware to set the tags when a note is selected

This avoids the ugly code in the NoteTextComponent where we determine
if tags are to be fetched, identify if they have been modified, fetch
them and then dispatch an action to update the store which might
again re-render the component.

Also implements style related fixes from #1000

Fixes: #469

Tests

  • Creating a new notebook should work as expeted without any issues.
  • Adding a tag to a new note should display the tag under the note.
  • Updating an existing tag, should update the tag under the note.
  • Deleting a tag removes it from the note.
  • Adding a new tag to a note should display the tag under the note.
  • Renaming a tag changes the tag in the note.
  • Tags should appear in the same order on the dialog box and on the note.
  • A new note should not display the tag bar.
  • Tag list order under note should be in ascending order for now.
  • Switching between notes updates the tag bar with tags for the respective notes.
  • Proper margin between toolbars when,
    • Tags are not present.
    • Tags are present.
  • During testing the CPU usage by Joplin should not peak at 100% for one or more CPU cores.

action.type === 'NOTE_SELECT_TOGGLE') {
let noteTags = [];
// We don't need to show tags unless only one note is selected.
if (newState.selectedNoteIds && newState.selectedNoteIds.length === 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurent22 - I've made this fairly restrictive since we only display note contents when a single tag is selected.

@tessus tessus added the desktop All desktop platforms label Dec 23, 2019
Follow up to #893

Now using middleware to set the tags when a note is selected

This avoids the ugly code in the NoteTextComponent where we determine
if tags are to be fetched, identify if they have been modified, fetch
them  and then dispatch an action to update the store which might
again re-render the component.

Also implements style related fixes from #1000

Signed-off-by: Abijeet <abijeetpatro@gmail.com>
Fixes: #469
@Abijeet Abijeet marked this pull request as ready for review December 25, 2019 14:42
@Abijeet
Copy link
Contributor Author

Abijeet commented Jan 6, 2020

Ping @laurent22

@Abijeet Abijeet changed the title Display selected tags under a note title Desktop: Display selected tags under a note title Jan 6, 2020
@laurent22
Copy link
Owner

@Abijeet, sorry for the late reply, I've only just started to go through the open pull requests.

Couldn't find any issue with your changes, it all looks very solid, so we can merge. Thanks for this feature, I think many people will like that!

@laurent22 laurent22 merged commit ae3a278 into laurent22:master Jan 6, 2020
@Abijeet
Copy link
Contributor Author

Abijeet commented Jan 7, 2020

Sorry for the late reply, I've only just started to go through the open pull requests.

No problem. Just wanted to ensure that this did not miss your radar considering you probably get 10's of notification daily.

Happy new year btw!

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.

[Feature Request] Display assigned tags at Note edit/view pane
3 participants