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

Make apps settings tabular #3195

Merged
merged 19 commits into from
Apr 25, 2017
Merged

Make apps settings tabular #3195

merged 19 commits into from
Apr 25, 2017

Conversation

eppfel
Copy link
Member

@eppfel eppfel commented Jan 22, 2017

Based on #2533 (comment) I introduced a table for the installed apps and removed Authors and description from the apps, because this view is mostly for managing your installation. (And shortened the "Enable only for specific groups" -> "Limit to groups")

I introduce a category "All installed" and set it as default. This view is sorted by

  1. enabled > disabled
  2. level (official > not)
  3. natural (name, ...)

We have to discuss which info is essential (Link to store/docs/repo/...). For now I introduced a link to the store, but this breaks on apps, which are not from the store. How do I get this data from an App? Internal only works to get apps, which are delivered with Nextcloud.

  • Which info to show and how (view in store)
  • Uninstall does not remove the app properly from the DOM
  • icons are not perfectly in the middle, yet
  • tooltip for enable-groups
  • Tests
  • Introduce a proper table header (we could use this to remove the "limit to groups" completely)

This is only about the tabular redesign of the installed apps. See #3194 for the complete apps management redesign.

@nextcloud/designers

@eppfel
Copy link
Member Author

eppfel commented Jan 22, 2017

app-tabular kopie

@skjnldsv
Copy link
Member

skjnldsv commented Jan 22, 2017

@eppfel Nice for an easy search. You may want to reduce the opacity of icons to 0.5 though :)
c3163cf8-e0c8-11e6-894f-b412ca598037

-ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=50)";
filter: alpha(opacity=50);
opacity: .5;
color: rgba(85,85,85,.5);
Copy link
Member Author

Choose a reason for hiding this comment

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

@skjnldsv any concerns with this?

Copy link
Member

Choose a reason for hiding this comment

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

Why not use opacity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to change this to color: rgba($color, .5) with the switch to scss

Copy link
Member Author

Choose a reason for hiding this comment

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

The border is transparent as well then

Copy link
Member

Choose a reason for hiding this comment

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

Where will the $color var comes from?

I havent implemented the variables yet :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not plan to switch to scss with this PR 😁 Just was not sure, if it safe to switch

Copy link
Member

Choose a reason for hiding this comment

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

Test and we'll see :p

Copy link
Member

Choose a reason for hiding this comment

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

hum, since this is located outside core/css it won't work.
The function isn't implemented yet.

It's planned though :)

Copy link
Member

Choose a reason for hiding this comment

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

Hum bis: in fact it will work. But I cant't guarantee that it will properly yet :/

@jancborchardt
Copy link
Member

jancborchardt commented Jan 23, 2017

Hmm, I’m not entirely sure about this since we will want to make screenshots more important in the future. Also it corresponds to a grid view that we might want to introduce for the Files app.

What do you think here @LukasReschke @MorrisJobke @BernhardPosselt?

@jancborchardt
Copy link
Member

I introduce a category "All installed" and set it as default.

The default view should be a new category »All apps«. Changing category just for installing apps is what’s already cumbersome right now. ;)

@eppfel eppfel mentioned this pull request Jan 23, 2017
13 tasks
@eppfel
Copy link
Member Author

eppfel commented Jan 23, 2017

Just to make this clear again. This is only for installed apps. Appstore categories will be in another PR (see #3194) and I am positive on the larger screenshots.

Hmm, I’m not entirely sure about this since we will want to make screenshots more important in the future.

I think we have to acknowledge that there are two major use-cases.

  1. Installing new apps
    1. explorative: What (new) apps are there?
    2. targeted: I want to install x?
  2. Manage apps
    1. targeted: App x is not useful/broken/etc. and I want to remove it?
    2. targeted: The updater disabled some Apps, let's see and reenable.
    3. targeted: There are updates, do them.

I think the explorative use is very seldom and you might do this in the appstore right for now.
The targeted new install will probably be by a search in an "All available apps" category (see #3194 ).

All managing use-cases are (mostly) directed and in my experience happen more often. The tabular view is just too show more apps in one view. And screenshots not add anything here, because you already know the apps and probably remember their icons.

Mac app store for example:

appstore-explore
appstore-bought

The default view should be a new category »All apps«. Changing category just for installing apps is what’s already cumbersome right now. ;)

"All installed": The idea is to manage installed apps in one place, rather than two (disabled / enabled).
We can introduce the "All available" in a next step and set it default then, but still I would like to introduce "All installed", because with more and more apps in the store, these might disturb the user. What's your take on that @jancborchardt ?

Further. installing new apps happens less often, so another click is unproblematic IMO. And once selected the category will not change. So even if you install a bunch of new apps on a new installation, you hit "All available" once and then search the apps you need one after another.

And I need feedback on leaving out meta, like author aso.

@Espina2
Copy link
Contributor

Espina2 commented Jan 23, 2017

This is very good. The offical tag is just weird, it looks like a button. Can we transform this in a normal text style? @eppfel @jancborchardt

@eppfel eppfel force-pushed the settings-apps-tabular branch from c3d5aa3 to 35ee78c Compare January 23, 2017 15:30
@jancborchardt
Copy link
Member

@eppfel ok, agree on the layout. I thought about it more too, and thanks a lot for the added design reasoning. :)

"All installed": The idea is to manage installed apps in one place, rather than two (disabled / enabled).

Installed == enabled though. We should not introduce another word or concept here. Maybe we should switch the wording from »Enabled / Not enabled« to »Installed / Not installed« (and »Install / Uninstall« on the buttons) but definitely not use both words at the same time.

Further. installing new apps happens less often, so another click is unproblematic IMO.

Installing new apps is definitely as big of a use case as managing the current ones is. See also the existing issue: »All apps« category in apps management #126

Another thing btw talking about simplifying and streamlining the management: We should link from the list of installed apps to the settings section of each app. :) Also cc @nickvergessen

@eppfel
Copy link
Member Author

eppfel commented Jan 23, 2017

That's how it looks right now.
bildschirmfoto 2017-01-23 um 17 49 41

The offical tag is just weird, it looks like a button. Can we transform this in a normal text style?

@Espina2 Yes, this needs rework. We might want to introduce batches/chips?
But the question is, do we need this info there anyway?

bildschirmfoto 2017-01-23 um 17 58 17

Installed == enabled though. We should not introduce another word or concept here. Maybe we should switch the wording from »Enabled / Not enabled« to »Installed / Not install

@jancborchardt
But technically there is a difference.

  • Installed / not installed: is the app present on your installation or not
  • enabled / not enabled: is the app active and can be used

I believe admins care for the difference.

After reading #126 "All installed" does exactly this (even the sorting). But I guess you want the appstore categories included as well, right?

I move the discussion about the apps management structure / navigation / categories to the overview issue #3194

@Espina2
Copy link
Contributor

Espina2 commented Jan 23, 2017

Another problem is the iconography used to represent the apps is not unique enough. We already use the same icons to others things... But in the other hand I know its hard to have more unique icons on this...

Anyway its looks a lot better @eppfel .

@eppfel
Copy link
Member Author

eppfel commented Jan 25, 2017

Ok, I want to get this in, so let's warp things up.

warp speed

I'll postpone:

  • Which info to show and how (view in store) -> Bigger issue and should be easy to integate later into the tabular layout
  • Introduce table header -> for now columns are self-explanatory
  • tooltip for enable-groups -> kind of dependents on the two previous points

The two questions remaining is

1. the navigation:

I propose: Drop enabled and disabled and make it into one category: "Enabled/Disabled" or "Installed". Either way, it is an improvement to have all installed apps in one view. See #3194 for broader discussion.

2. dynamic sorting

When enabling an app should the list be sorted and the app jump up. I say we leave the app, where it is, to not confuse the user.

@claell
Copy link

claell commented Jan 25, 2017

  1. I agree that your proposal of the category "installed" offers the same functionality as "enabled" and "disabled". So they can be dropped.
    However I'd change the name to not confuse the user to something like "downloaded". Installed means the same as enabled to me.
    This clarifies the whole situation a bit more. When I first used the app store I did not know what exactly "enabled" and "disabled" is and that there can be downloaded but not enabled apps.

  2. How are the apps sorted right now? Alphabetically? It might be helpful to have some order and having all enabled apps on top might lead to better usability. If this is not done dynamically I'd propose to leave the position where it is when inside the category and show it resorted on next visit.

eppfel and others added 5 commits April 25, 2017 00:22
Signed-off-by: Felix A. Epp <work@felixepp.de>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt force-pushed the settings-apps-tabular branch from 87fd553 to 88bc431 Compare April 24, 2017 22:23
@jancborchardt
Copy link
Member

All issues fixed and rebased too. Ready for review @nextcloud/javascript @nextcloud/designers! :)

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

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍 I restarted the CI because JS unit tests failed with a connection timeout.

@MorrisJobke
Copy link
Member

Mmmmh ... "/drone/src/github.com/nextcloud/server/core/vendor/DOMPurify/dist/purify.min.js" were excluded or matched by prior matchers.

@LukasReschke You touched this lately. Any idea?

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks great!

Only two remarks:

  1. "Limit to groups" is not aligned with the checkbox
    bildschirmfoto von 2017-04-25 08-13-25

  2. During enabling/disabling an app, the button text is wider and therefore causes the columns to change their size. This look a bit odd.

@jancborchardt
Copy link
Member

@ChristophWurst

"Limit to groups" is not aligned with the checkbox

This seems to be an issue in master actually. Will see to fix via a different PR.

During enabling/disabling an app, the button text is wider and therefore causes the columns to change their size. This look a bit odd.

Yes, but this only occurs on narrow sizes and will be variable with languages anyway. There’s no immediate way I can think of adjusting that. We should check it out after feature freeze, ok? :)

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Fine by me

@LukasReschke
Copy link
Member

Mmmmh ... "/drone/src/github.com/nextcloud/server/core/vendor/DOMPurify/dist/purify.min.js" were excluded or matched by prior matchers.

@LukasReschke You touched this lately. Any idea?

Not a lot of ideas since it works locally but #4486 could be it.

@MorrisJobke
Copy link
Member

Okay - the JSUnit failure is unrelated -> merge it.

@MorrisJobke MorrisJobke merged commit 6f2df5e into master Apr 25, 2017
@MorrisJobke MorrisJobke deleted the settings-apps-tabular branch April 25, 2017 13:25
@MariusBluem
Copy link
Member

Awesome 🎉🙈

@jancborchardt
Copy link
Member

Awesome work @eppfel! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants