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

Click a note link will now select the right folder note and not /home #2941

Merged
merged 3 commits into from
Mar 25, 2019

Conversation

dredav
Copy link
Contributor

@dredav dredav commented Mar 22, 2019

Description

After following a link to an other note the folder /home (All Notes) will be selected. The expected behavior is that the storage folder of the requested note will be selected.

Screenshot before

Kapture 2019-03-22 at 7 30 09

Screenshot after

Kapture 2019-03-22 at 7 32 14

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

@dredav
Copy link
Contributor Author

dredav commented Mar 22, 2019

This pull request will also fix the issues #2823 and #2909

##Screenshot after
Kapture 2019-03-22 at 7 42 32

this.focusNote(selectedNoteKeys, noteHash, '/home')

let locationToSelect = '/home'
const notesByHash = data.noteMap.map((note) => note).filter((note) => note.key === noteHash)
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use find instead of filter? Can you explain it to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG Thanks for your Feedback. I was inspired by the handleNoteClick function in NoteList/index.js. I've changed the code and using the find function to get the first note and not all notes that matching the key (which always will be one).

@ZeroX-DG ZeroX-DG added the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Mar 22, 2019
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.

I'm sorry but I just can't hold it. Sometimes, I think I have OCD 😄

browser/main/NoteList/index.js Outdated Show resolved Hide resolved
Co-Authored-By: dredav <dredav@users.noreply.github.com>
@dredav
Copy link
Contributor Author

dredav commented Mar 23, 2019

I'm sorry but I just can't hold it. Sometimes, I think I have OCD 😄

@ZeroX-DG Thanks for you suggestion. I have applied it.

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

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Mar 24, 2019
@Rokt33r Rokt33r changed the title Klick a note link will now select the right folder note and not /home Click a note link will now select the right folder note and not /home Mar 25, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Mar 25, 2019
@Rokt33r Rokt33r added this to the v0.11.15 milestone Mar 25, 2019
@Rokt33r Rokt33r merged commit 5b99132 into BoostIO:master Mar 25, 2019
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.

3 participants