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

Assign users to cards #314

Merged
merged 21 commits into from
Oct 23, 2017
Merged

Assign users to cards #314

merged 21 commits into from
Oct 23, 2017

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 1, 2017

bildschirmfoto vom 2017-10-01 17-13-40

This implements #11

TODO:

@codecov
Copy link

codecov bot commented Oct 10, 2017

Codecov Report

Merging #314 into master will increase coverage by 1.38%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   76.83%   78.22%   +1.38%     
==========================================
  Files          36       38       +2     
  Lines        1131     1194      +63     
==========================================
+ Hits          869      934      +65     
+ Misses        262      260       -2

@juliusknorr juliusknorr force-pushed the assign-users branch 3 times, most recently from f248e8f to 7c8532c Compare October 11, 2017 12:56
@juliusknorr
Copy link
Member Author

This is ready for testing/review. @pixelipo @artemanufrij Would be cool if you could give it a try.

Also cc @terrar @ossie @poVoq

@pixelipo pixelipo self-requested a review October 13, 2017 10:19
Copy link
Contributor

@pixelipo pixelipo left a comment

Choose a reason for hiding this comment

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

Works quite nicely! Good work, @juliushaertl

I've just made a few small comments regarding CSS

css/style.scss Outdated
background: $color-lightgrey;
display: flex;
position: relative;

.card-options {
opacity: 0.3;
Copy link
Contributor

Choose a reason for hiding this comment

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

we still need this one, I think (but maybe it should be 0.5 instead).

@@ -18,7 +18,9 @@
<div class="tabsContainer">
<div id="commentsTabView" class="tab commentsTabView" ng-if="status.boardtab==0 || !status.boardtab">

<ui-select ng-if="boardservice.canShare()" ng-model="status.addSharee" theme="select2" style="width:100%;" title="Choose a user to assign" placeholder="Assign users ..." on-select="aclAdd(status.addSharee)" search-enabled="true">
<ui-select ng-if="boardservice.canShare()" ng-model="status.addSharee" theme="select2" style="width:100%;"
title="Choose a user to assign" placeholder="Assign users ..."
Copy link
Contributor

Choose a reason for hiding this comment

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

missing translation strings

@@ -18,7 +18,9 @@
<div class="tabsContainer">
<div id="commentsTabView" class="tab commentsTabView" ng-if="status.boardtab==0 || !status.boardtab">

<ui-select ng-if="boardservice.canShare()" ng-model="status.addSharee" theme="select2" style="width:100%;" title="Choose a user to assign" placeholder="Assign users ..." on-select="aclAdd(status.addSharee)" search-enabled="true">
<ui-select ng-if="boardservice.canShare()" ng-model="status.addSharee" theme="select2" style="width:100%;"
Copy link
Contributor

Choose a reason for hiding this comment

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

is the inline style (width) really needed here?

css/style.scss Outdated
display: flex;
margin-bottom: 5px;
margin-top: 20px;
min-height: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

is min-height really needed?

css/style.scss Outdated
h4 {
padding-top: 5px;
padding-bottom: 5px;
border: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed?

css/style.scss Outdated
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty lines

@pixelipo
Copy link
Contributor

@juliushaertl I made a couple of commits here, I hope you don't mind. I think they make the whole .card-controls bar a bit cleaner and more consistent:

  • it's actually 2px smaller now, to a standard 44px used throughout NC
  • .icon-more icon is now more touchscreen-friendly, with and activation area of 44x44px
  • avatars are 32px standard size without affecting overall height

@juliusknorr juliusknorr mentioned this pull request Oct 16, 2017
4 tasks
@juliusknorr
Copy link
Member Author

Thanks for the commits. There is a small issue when no card description is set. I'll take care of that.

@juliusknorr juliusknorr self-assigned this Oct 16, 2017
@juliusknorr juliusknorr removed their assignment Oct 18, 2017
@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 18, 2017

@pixelipo Fixed. Thank you.

@ossie
Copy link
Contributor

ossie commented Oct 22, 2017

@juliushaertl I checked out pr #314 and tried to assign users: Fetching the user list fails.
I keep getting JS errors: [$injector:unpr] http://errors.angularjs.org/1.6.4/$injector/unpr?p0=withoutAssignedUsersFilterProvider%20%3C-%20withoutAssignedUsersFilter.
Users assigned directly in the database look fine, though.

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>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr and others added 16 commits October 23, 2017 21:18
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>
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>
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: Marin Treselj <marin.treselj@forlagshuset.no>
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@ossie Thanks for giving it a try. That migh be caused when trying it before 9b54d1c#diff-70c43e8d89ea7f7d5e64f83246f17ed0R50 was added to the PR, and running in debug mode. At least i could not reproduce it anymore. Let's merge now. Feel free to open an issue if that sill occurs on master.

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.

3 participants