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

Add Updates section to app management #6739

Merged
merged 5 commits into from
Oct 10, 2017
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Oct 3, 2017

Add updates section to the apps management (fixes #838)

bildschirmfoto vom 2017-10-03 14-33-08

Partly fixes #301 since it removes the notification in the header and adds a badge with the update count to the sidebar.

@codecov
Copy link

codecov bot commented Oct 3, 2017

Codecov Report

Merging #6739 into master will decrease coverage by 0.05%.
The diff coverage is 35.21%.

@@             Coverage Diff             @@
##             master   #6739      +/-   ##
===========================================
- Coverage     53.05%     53%   -0.06%     
+ Complexity    22650   22614      -36     
===========================================
  Files          1430    1422       -8     
  Lines         88062   88023      -39     
  Branches       1343    1346       +3     
===========================================
- Hits          46725   46658      -67     
- Misses        41337   41365      +28
Impacted Files Coverage Δ Complexity Δ
settings/templates/apps.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/legacy/app.php 54.23% <100%> (+0.91%) 218 <0> (-4) ⬇️
settings/js/apps.js 24.57% <30.23%> (+0.17%) 0 <0> (ø) ⬇️
settings/Controller/AppSettingsController.php 19.2% <52.38%> (+3.04%) 71 <5> (+6) ⬆️
...es_sharing/lib/Controller/ShareesAPIController.php 64.47% <0%> (-5.53%) 114% <0%> (+90%)
apps/files/templates/list.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/contactsmenu.js 91.97% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files/templates/simplelist.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
core/templates/layout.user.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 15 more

@juliushaertl juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 3, 2017
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

After an update, the counter is set to -1! :)
Otherwise, great addition! Works nicely!

capture d ecran_2017-10-03_16-24-17

@juliushaertl
Copy link
Member Author

@skjnldsv Thanks. Fixed with the latest commit.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Message for no updates available is not good! :)

capture d ecran_2017-10-03_17-01-33

Also, I think we should hide the section if no updates are available :)

@juliushaertl
Copy link
Member Author

@skjnldsv I'd prefer to keep the section even if it is empty. That way users might remember where to check for updates if they come back.

This is how it looks now, when there are no updates available:
bildschirmfoto vom 2017-10-03 18-11-56

@skjnldsv
Copy link
Member

skjnldsv commented Oct 3, 2017

Can you add a manual trigger button instead of the zero count then? I don't see why a user would click this section if the zero is clearly visible! :)

@juliushaertl
Copy link
Member Author

juliushaertl commented Oct 3, 2017

Hm maybe we should hide it then. There is a background job for fetching updates and I don't think we need an option to manually check. Any other thoughts on that @nextcloud/designers

$('#apps-categories').html(html);
$('#app-category-' + OC.Settings.Apps.State.currentCategory).addClass('active');
if (updateCategory.length === 1) {
console.log(updateCategory);
Copy link
Member

Choose a reason for hiding this comment

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

🙈

@nickvergessen
Copy link
Member

Hm maybe we should hide it then.

I agree, hide the section when empty (just like play store and others do it as well)
Makes little sense to have it, when there are no updates pending

@juliushaertl
Copy link
Member Author

@nickvergessen @skjnldsv Fixed. The updates category is now hidden. It will be shown if an update get available while the user is browsing the page.

@jancborchardt
Copy link
Member

@juliushaertl Actually, and sorry I only realize this now – with »Show apps with available updates first« #6740 by you this seems to be not necessary anymore. :)

I would prefer if it’s nicely integrated simply by sorting up, and not with the additional view. It’s duplicated otherwie. Sorry for the late realization, but thanks for the two great pull requests! :) What do you think?

@skjnldsv
Copy link
Member

skjnldsv commented Oct 4, 2017

@jancborchardt I don't mind the extra section. I find it easier to search what you need. Users can't directly know that the updates are on top of the list, while with the section, there is no confusion possible.

@jancborchardt
Copy link
Member

Hm, then maybe we should add a heading, like for the bundles? I really think that we should keep the navigation lean and not list everything possible.

@jancborchardt
Copy link
Member

Cause that will make the list clearer in general – also for »Enabled apps« and »Disabled apps«. Because currently they are sorted properly, but don’t have any section separation which would be nice. Then we could also simply cut those sidebar entries out and just have »Your apps«.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 4, 2017

Still disagree with that (sorry! 🙈). I dislike having to scroll to search. Restricting views are a big plus on this. I rather click to se that scroll x time to get to the section I need. I have a lot apps installed. :)

@juliushaertl
Copy link
Member Author

Hm, then maybe we should add a heading, like for the bundles? I really think that we should keep the navigation lean and not list everything possible.

That really depends on how much apps you have installed/enabled. You never realize if you have a long list that you need to scroll down to find disabled apps there. A quick filter in the sidebar gives the user a hint that there is other stuff to look for.

@jancborchardt
Copy link
Member

Ok, but for apps with an update I think it's still valid. There will likely never be so many apps at the same time with an update that you need to scroll.

But my feelings on this aren't soo strong that I feel arguing against you more, so ok. ;)

@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 8, 2017
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit 0def38b into master Oct 10, 2017
@juliushaertl juliushaertl deleted the apps-management-updates branch October 10, 2017 12:57
@hanzei hanzei mentioned this pull request Oct 19, 2017
13 tasks
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Oct 23, 2017
@georgehrke
Copy link
Member

This pull-request doesn't seem to introduce an Update all button. Is there a dedicated pr or issue for that? :)

@juliushaertl
Copy link
Member Author

This pull-request doesn't seem to introduce an Update all button. Is there a dedicated pr or issue for that? :)

Not that I'm aware of, but that is a neat idea.

@jancborchardt
Copy link
Member

@georgehrke @juliushaertl mind opening a new isue for it? Or is anyone already working on a pull request? :)

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: apps management
Projects
None yet
6 participants