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.

Sign up for GitHub

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

Jump to bottom

Show only related tags, select multiple tags #1775

Merged
merged 8 commits into from
Apr 26, 2018
Merged

Show only related tags, select multiple tags #1775

merged 8 commits into from
Apr 26, 2018

Conversation

bimlas
Copy link
Contributor

@bimlas bimlas commented Apr 1, 2018

When a tag is selected, the tag list narrows to show only the related
ones: all tags associated to the currently visible notes. Clicking on
the plus sign near another tag narrows the list again to the tags of
notes associated with the firstly AND secondly selected tag. To show
every tags again, press the tag icon on the top-left corner of
Boostnote.

Before:
screencast

After:
screencast

NOTE: Tags are joined with & character (# not works) in
location.pathname thus it will make the tags with this character
unavailable. Any suggestion to pass multiple values via pathname?

Please check browser/components/TagListItem.styl, I'm sure that it can be simpler.

Related issues:

@Rokt33r Rokt33r added feature request 🌟 Issue is a new feature request. awaiting review ❇️ Pull request is awaiting a review. and removed feature request 🌟 Issue is a new feature request. labels Apr 2, 2018
@bimlas
Copy link
Contributor Author

bimlas commented Apr 7, 2018

I just changed to use character to separate tags in path (/tags/currently selected tags) instead of & (/tags/currently&selected&tags) to prevent interference with tags containing this char (black&white). See commit message. I think it's stable enough to get merged.

@sosukesuzuki sosukesuzuki mentioned this pull request Apr 10, 2018
Closed
@bimlas
Copy link
Contributor Author

bimlas commented Apr 10, 2018 •
edited
Loading

Changed the behaviour: does not narrowing the list.

screencast

@bimlas bimlas force-pushed the narrow-to-related-tags branch 2 times, most recently from 69c3f8c to 9f21192 Compare April 11, 2018 06:42
@bimlas
Copy link
Contributor Author

bimlas commented Apr 11, 2018 •
edited
Loading

Just made the final version:

screencast

@bimlas
Copy link
Contributor Author

bimlas commented Apr 11, 2018

If it's ok, then I'll try to write tests.

@bimlas bimlas mentioned this pull request Apr 11, 2018
Closed
@bimlas bimlas changed the title Narrow list of tags to related ones Show only related tags, select multiple tags Apr 11, 2018
@sosukesuzuki sosukesuzuki self-requested a review April 11, 2018 09:10
sosukesuzuki
sosukesuzuki requested changes Apr 11, 2018
View reviewed changes
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

please confirm my review!

browser/main/index.js Outdated
@@ -73,6 +73,9 @@ ReactDOM.render((
<IndexRedirect to='/alltags' />
<Route path=':tagname' />
</Route>
<Route path='narrowToTag'>
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just forgot to remove, thanks for pointing it out!

@sosukesuzuki
Copy link
Member

LGTM:smile:

@bimlas
Copy link
Contributor Author

bimlas commented Apr 11, 2018

Should I write tests in a different PR or this one is not finished without those?

sosukesuzuki
sosukesuzuki requested changes Apr 11, 2018
View reviewed changes
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I forgot about tests.
please write tests in this pr.

@bimlas
Copy link
Contributor Author

bimlas commented Apr 11, 2018

I'm so sorry, but it's too hard for me: Boostnote is the very first React / Electron / whatever project for me and I'm just a beginner in the Javascript world, thus writing such tests is nearly impossible. All what I did is modifying Jest test.

@Rokt33r
Copy link
Member

Rokt33r commented Apr 12, 2018

@bimlas I'll take care of it. 😄

@sosukesuzuki sosukesuzuki removed the awaiting review ❇️ Pull request is awaiting a review. label Apr 24, 2018
@kazup01 kazup01 added the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Apr 26, 2018
@kazup01
Copy link
Member

kazup01 commented Apr 26, 2018

Hi @bimlas , thanks for your contribution! There is conflict, could you fix it?

@bimlas
Narrow list of tags to related ones
6c54275 
When a tag is selected, the tag list narrows to show only the related
ones: all tags associated to the currently visible notes. Clicking on
the plus sign near another tag narrows the list again to the tags of
notes associated with the firstly AND secondly selected tag. To show
every tags again, press the tag icon on the top-left corner of
Boostnote.

Before:
![screencast](https://i.imgur.com/PwAdhLe.gif)

After:
![screencast](https://i.imgur.com/s3JCaFq.gif)

NOTE: Tags are joined with `&` character (`#` not works) in
`location.pathname` thus it will make the tags with this character
unavailable. Any suggestion to pass multiple values via pathname?
@bimlas
Separate active tags by instead of &
68c0f21 
Using `&` to separate tags in path (like
`/tags/currently&selected&tags`) may interfer with tags including `&`
character (like `black&white`). Since ` ` is replaced with `_` when
adding tag to notes, it's ideal separator because it's guaranteed that
tags are not including this character.
@bimlas
Don't narrow list, add plus sign only for related tags
61ed47d 
It's easier to understand by most of the users.

Later I like to add a setting to enable narrowing of tag list to show
only the related ones.
@bimlas
Cleanup, prettify
066d972
@bimlas
Simplify style
c02b91d
@bimlas
Copy link
Contributor Author

bimlas commented Apr 26, 2018

Rebased, fixed.

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Apr 26, 2018
@sosukesuzuki sosukesuzuki removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. awaiting review ❇️ Pull request is awaiting a review. labels Apr 26, 2018
@kazup01 kazup01 merged commit b191213 into BoostIO:master Apr 26, 2018
@kazup01
Copy link
Member

kazup01 commented Apr 26, 2018 •
edited
Loading

Merged. Thank you for your contribution @bimlas !

@bimlas bimlas deleted the narrow-to-related-tags branch May 4, 2018 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sosukesuzuki sosukesuzuki sosukesuzuki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bimlas @sosukesuzuki @Rokt33r @kazup01