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

Fix: Add selected tag to new notes by default #2556

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

codebykat
Copy link
Member

@codebykat codebykat commented Jan 8, 2021

Fix

Fixes #2423

If you are on a selected tag and choose to add a new note, the new note will now automatically get that tag added to it.

Test

  1. Click a tag in the sidebar to select it
  2. Create a new note
  3. Verify that the new note has the selected tag
  4. Create a new note with no tag selected
  5. Verify that no tags are added to that note

Release

Fix: Apply selected tag to a new note by default

@codebykat codebykat added this to the 2.6.0 milestone Jan 8, 2021
Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Pretty straight-forward code. You know me: I don't like let or mutation so I left a comment about removing it but the logic looks good. 👍

@codebykat
Copy link
Member Author

@dmsnell I appreciate the review but there's a major bug with this as noted in the TODO, which is why it was still set to draft. Do you know what I need to pass to get it to work with a tag with special characters in its name?

@dmsnell
Copy link
Contributor

dmsnell commented Jan 10, 2021

since we're writing the tags into the tags array on the note object we shouldn't have to worry about transformation; we should be able to keep it written as typed in the search bar.

withTag should be preserving whatever it's given. when you say "empty" tag can you share the data you are referring to? what makes it empty vs. not having the tag at all?

@codebykat
Copy link
Member Author

I haven't debugged it deeply yet, but the note shows up with an empty tag pill.

@codebykat codebykat modified the milestones: 2.6.0 ❄, 2.8.0 Feb 9, 2021
@codebykat codebykat modified the milestones: 2.8.0, 2.9.0 Mar 10, 2021
@sandymcfadden
Copy link
Contributor

This seems to be all working now. I've updated to get the tagName of the openedTag before sending to withTag as it was expecting a tagName and not a tagHash.

@sandymcfadden sandymcfadden requested a review from a team March 25, 2021 17:50
@sandymcfadden sandymcfadden marked this pull request as ready for review March 25, 2021 17:50
@dmsnell
Copy link
Contributor

dmsnell commented Mar 25, 2021

we need to rebase this against the latest develop HEAD. state.ui.openedTag is gone and has been replaced by state.ui.collection in #2706, but which thankfully introduced a new openedTag selector as well so we should be able to easily adapt this branch.

@sandymcfadden
Copy link
Contributor

@dmsnell ah yes you're right. I was going to rebase before I merged. I forgot that change would impact this. I'll work on that now.

@sandymcfadden
Copy link
Contributor

Thanks again @dmsnell for that catch. All updated again now.

Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

there's nothing I see worth pointing out; it's a clean solution

for testing, did you verify that the added tag is the name of the tag as pulled from the tag bucket, not the tag hash, not a trivial transformation from the tag hash to an unhashed version of the tag? (for example, does it properly at Tag if Tag is opened, because tag is the tag hash there).

Copy link
Member Author

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

[Can't request changes because I'm the author apparently 😂 ]

There's an oddity that seems to be resetting the notes list to "all notes" after a new note is added. It's still displaying the tag in the title bar, but the note listing is now showing everything, not just notes with that tag:

Screen Shot 2021-03-25 at 6 23 49 PM

Screen Shot 2021-03-25 at 6 23 55 PM

@sandymcfadden
Copy link
Contributor

for testing, did you verify that the added tag is the name of the tag as pulled from the tag bucket, not the tag hash, not a trivial transformation from the tag hash to an unhashed version of the tag? (for example, does it properly at Tag if Tag is opened, because tag is the tag hash there).

Thanks @dmsnell! The openedTag selector provides us with the tagName, withTag wants the tagName as well and then tests to see if the hash of it matches the hashes of any other tags in the list already before adding it in.
I did test with the example you gave and Tag is added to the note.

@sandymcfadden
Copy link
Contributor

There's an oddity that seems to be resetting the notes list to "all notes" after a new note is added. It's still displaying the tag in the title bar, but the note listing is now showing everything, not just notes with that tag:

Thanks @codebykat! I didn't notice that at all. I've got it updated now so that it displays the note list with the selected tag properly.

@codebykat
Copy link
Member Author

This is my PR so I can't approve it, but this is an approval! It's testing great for me now.

@sandymcfadden sandymcfadden modified the milestones: 2.9.0, 2.10.0 Mar 30, 2021
@sandymcfadden sandymcfadden self-assigned this Mar 31, 2021
@sandymcfadden sandymcfadden force-pushed the fix/add-selected-tag-to-new-note branch from e10517f to c856a59 Compare April 8, 2021 12:14
@sandymcfadden sandymcfadden merged commit 21bd75e into develop Apr 8, 2021
@sandymcfadden sandymcfadden deleted the fix/add-selected-tag-to-new-note branch April 8, 2021 12:34
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.

Tag isn't added automatically when a tag is selected from the menu
3 participants