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

Move lately used tags to the top #1934

Merged
merged 4 commits into from
Dec 5, 2016
Merged

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Oct 27, 2016

  • Add a tag to a file
  • Open the sidebar of another file
  • See the tag you just assigned being sorted first.

Stores up to 10 tags atm
Ref 2nd checkbox of #1465

@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81 to be a potential reviewer.

@@ -57,6 +57,8 @@

_newTag: null,

_lastUsedTags: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this file is in core, instead of systemtags app.

But maybe the sorting should be moved to dav then aswell

@codecov-io
Copy link

codecov-io commented Oct 27, 2016

Current coverage is 57.78% (diff: 0.00%)

Merging #1934 into master will decrease coverage by 0.02%

@@             master      #1934   diff @@
==========================================
  Files          1094       1096     +2   
  Lines         62800      62825    +25   
  Methods        6996       6999     +3   
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          36303      36303          
- Misses        26497      26522    +25   
  Partials          0          0          
Diff Coverage File Path
0% new ...pps/systemtags/lib/Controller/LastUsedController.php
0% new apps/systemtags/appinfo/routes.php
0% apps/systemtags/lib/Activity/Listener.php
•••••••••• 100% .../lib/SystemTag/SystemTagsObjectMappingCollection.php

Powered by Codecov. Last update 1857e50...8e5651c

@MorrisJobke MorrisJobke changed the title Move latly used tags to the top Move lastly used tags to the top Nov 2, 2016
@MorrisJobke MorrisJobke changed the title Move lastly used tags to the top Move lately used tags to the top Nov 2, 2016
@ChristophWurst
Copy link
Member

This only works if I reload the page after assigning the first tag. It looks like the ajax call to get the last used tags is only performed once.

@nickvergessen
Copy link
Member Author

Yes, it's only performed once, afterwards the local instance should keep it updated, or I did something wrong.

@ChristophWurst
Copy link
Member

afterwards the local instance should keep it updated

That does not work. I tested it with FF and without reloading the lastly assigned tag (I created a new one) was sorted as last tag in the list of suggested tags.

@nickvergessen
Copy link
Member Author

I created a new one

little important detail, will have a look

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Fixed

@nickvergessen nickvergessen force-pushed the move-latly-used-tags-to-the-top branch from f0541fd to 8e5651c Compare November 7, 2016 14:02
@MorrisJobke
Copy link
Member

Tested and works 👍

@MorrisJobke
Copy link
Member

@nextcloud/javascript @icewind1991 @rullzer @LukasReschke Please review

@rullzer
Copy link
Member

rullzer commented Nov 21, 2016

No tests?

@nickvergessen
Copy link
Member Author

Feel free to, I'm not good in JS tests

@jancborchardt
Copy link
Member

Awesome idea @nickvergessen!

@MorrisJobke
Copy link
Member

@rullzer Mind to review?

@MorrisJobke
Copy link
Member

@LukasReschke @ChristophWurst @rullzer Review?

@ChristophWurst
Copy link
Member

👍 now my test case worked 😉

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 5, 2016
@ChristophWurst
Copy link
Member

Btw. should we have a tooltip on the trash icon in the tag list? I had never used tags before and I wasn't sure whether the trash icon (only shown if one hits the pencil before) unassigns a tag or deletes it (which would also effect other files/folders).

@MorrisJobke MorrisJobke merged commit 8fdfb41 into master Dec 5, 2016
@MorrisJobke MorrisJobke deleted the move-latly-used-tags-to-the-top branch December 5, 2016 15:10
@MorrisJobke
Copy link
Member

Btw. should we have a tooltip on the trash icon in the tag list? I had never used tags before and I wasn't sure whether the trash icon (only shown if one hits the pencil before) unassigns a tag or deletes it (which would also effect other files/folders).

cc @nickvergessen @jancborchardt

@nickvergessen
Copy link
Member Author

Deleting should be removed altogether.
There is also an issue for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: tags
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants