juliushaertl / apporder

Nextcloud app to enable sorting inside the app menu
GNU Affero General Public License v3.0
45 stars 9 forks source link

Implementation of hide #40

Closed bakkegaard closed 7 years ago

bakkegaard commented 7 years ago

I would like some feedback on this code before I polish it up.

To do

juliushaertl commented 7 years ago

@bakkegaard Thanks, looks good so far except for some small comments :+1:

bakkegaard commented 7 years ago

Now it should be pretty much done, the only thing missing is the design. I have thought about it and think there are three ways we can go

  1. Copy the activity settings from above where first column is the checkbox, second is the icon and third is the text
    image
  2. Remove the checkbox and do so when you click on the icons the color flips together with the hidden/not hidden image
  3. A combination where we use design 1. but flips the color anyway.

Let me know what you think :)

juliushaertl commented 7 years ago

@bakkegaard Sorry, totally missed that comment. :wink:

I made a commit to fix the styling here: https://github.com/juliushaertl/apporder/commit/263ff18d07e8bf7c287af143d718bae258cacd13

Feel free to cherry-pick that commit into your pull request.

juliushaertl commented 7 years ago

This is how it looks like:

2017-07-06-125839_343x280_scrot

bakkegaard commented 7 years ago

No problem. Damn it looks good. Nice work. I will try to make a thorough test within the next couple of days to make sure everything works as expected. Feel free to do it yourself and merge, unless you have other comments on the code?

juliushaertl commented 7 years ago

@bakkegaard I found some bugs when testing this :wink:

Otherwise this works quite well. Great work so far. We can merge once these two errors are fixed ;)

bakkegaard commented 7 years ago

@juliushaertl should be fixed now :)

RainerEmrich commented 7 years ago

@bakkegaard , @juliushaertl I tested this version with nextcloud 12.0.1RC4. Looks really good and works AFAIS. It would be really nice if there will be a new release in the next days.

juliushaertl commented 7 years ago

@bakkegaard Thanks for this PR. Works really nice. Let's merge this so we can get a new release out.