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

Icon on apps settings #6627

Merged
merged 2 commits into from
Sep 25, 2017
Merged

Icon on apps settings #6627

merged 2 commits into from
Sep 25, 2017

Conversation

skjnldsv
Copy link
Member

Fix #3987

@skjnldsv skjnldsv added the 2. developing Work in progress label Sep 23, 2017
@skjnldsv skjnldsv self-assigned this Sep 23, 2017
@mention-bot
Copy link

@skjnldsv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @LukasReschke and @ChristophWurst to be potential reviewers.

@codecov
Copy link

codecov bot commented Sep 24, 2017 •
edited
Loading

Codecov Report

Merging #6627 into master will not change coverage.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #6627   +/-   ##
=========================================
  Coverage     53.06%   53.06%           
  Complexity    22553    22553           
=========================================
  Files          1414     1414           
  Lines         87745    87745           
  Branches       1340     1340           
=========================================
  Hits          46565    46565           
  Misses        41180    41180
Impacted Files Coverage Δ Complexity Δ
settings/templates/apps.php 0% <0%> (ø) 0 <0> (ø) ⬇️
settings/js/apps.js 24.29% <0%> (ø) 0 <0> (ø) ⬇️

@skjnldsv skjnldsv mentioned this pull request Sep 24, 2017
Closed
13 tasks
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 24, 2017
@skjnldsv
Copy link
Member Author

I tried to use most of stuff we already use on nextcloud to avoid confusion.
I created/edited some others. Please suggest a modification/new one if you don't like those.
There are some icons (like the key) which I don't like on my pr, I would be glad to see you suggest something else @nextcloud/designers :)

Thanks!
capture d ecran_2017-09-24_08-47-04

@skjnldsv
Icons & default classes
308e1c0 
Changed categories order same as app store

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
ChristophWurst
ChristophWurst previously requested changes Sep 25, 2017
View reviewed changes
settings/js/apps.js Outdated
{displayName: t('settings', 'Enabled apps'), ident: 'enabled', id: '0'},
{displayName: t('settings', 'Disabled apps'), ident: 'disabled', id: '1'},
{displayName: t('settings', 'Your apps'), ident: 'installed', id: '2'}
var categories = [,
Copy link
Member

Choose a reason for hiding this comment

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

remove the , please

settings/js/apps.js Outdated
{displayName: t('settings', 'Your apps'), ident: 'installed', id: '2'}
var categories = [,
{displayName: t('settings', 'Your apps'), ident: 'installed', id: '0'},
{displayName: t('settings', 'Enabled apps'), ident: 'enabled', id: '1',},
Copy link
Member

Choose a reason for hiding this comment

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

remove the , please

Copy link
Member Author

Choose a reason for hiding this comment

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

How did I missed that... 🙈
Thanks!!

@skjnldsv
Fix typo
9ff6dab 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
MorrisJobke
MorrisJobke approved these changes Sep 25, 2017
View reviewed changes
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.

Awesome 🚀 I like it

bildschirmfoto 2017-09-25 um 15 50 06

@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Sep 25, 2017
@MorrisJobke MorrisJobke mentioned this pull request Sep 25, 2017
Closed
@nickvergessen nickvergessen merged commit f2b7205 into master Sep 25, 2017
@nickvergessen nickvergessen deleted the icon-on-apps-settings branch September 25, 2017 14:29
@jancborchardt
Copy link
Member

Awesome stuff @skjnldsv I like it a lot! :)

The key is good too! Maybe only mirror it, so that the key ring is on the top right instead – that’s how keys are most often visualized.

@skjnldsv
Copy link
Member Author

@jancborchardt damn, few seconds too late! 😕
Feel free to provide a pr for that if you really want! 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews design Design, UI, UX, etc. feature: apps management
Projects
None yet
Milestone
Nextcloud 13
Development

Successfully merging this pull request may close these issues.
6 participants
@skjnldsv @mention-bot @jancborchardt @nickvergessen @MorrisJobke @ChristophWurst