Skip to content

Conversation

@raimund-schluessler
Copy link
Member

@raimund-schluessler raimund-schluessler commented Jul 21, 2020

Follow-up of #1135. Updates the style of the task list to a more friendly appearance. Please have a look @timkrief

tasklist

Also @jancborchardt and @nextcloud/designers for review.

Screenshot_2020-07-27 Tasks - Nextcloud

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #1136 into master will increase coverage by 50.93%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1136       +/-   ##
===========================================
+ Coverage   27.82%   78.76%   +50.93%     
===========================================
  Files          48        8       -40     
  Lines        2609      113     -2496     
  Branches      494        0      -494     
===========================================
- Hits          726       89      -637     
+ Misses       1743       24     -1719     
+ Partials      140        0      -140     

@raimund-schluessler
Copy link
Member Author

raimund-schluessler commented Jul 21, 2020

It looks nice already, but there are some visual issues:

  • On Firefox with 100% zoom level, some lines separating the tasks don't get rendered properly, see here
    grafik
    I think the issue is, that you use the subtasks-container to kind of hide the round corners when necessary. We should try to calculate whether a task item should have round corners and set the CSS accordingly. That means:

    • If a root task is the first task, it needs round corners at the top.
    • If a root task is the last task, it needs round corners at the bottom.
    • If a task no matter the level has subtasks shown, it needs a round corner at the bottom left and none at the bottom right.
    • Subtasks never have round corners at the top, but might have round corners at the bottom, if the task itself and all parent tasks are the last in their container.

    I think this should be doable with a bit of CSS and Javascript (to detect whether subtasks are visible). And it should work no matter the level, I hope.

  • The header of the task-details now merges with the new background-color, which looks weird.

  • I would prefer the calculation of the checkbox color https://github.com/nextcloud/tasks/pull/1135/files#diff-88cbe2e916273c5c244516d0ec6b33ffR530 to be done in Vue as we e.g. do it here https://github.com/nextcloud/tasks/blob/master/src/components/TheDetails.vue#L598.

  • You removed the background-color on hover and active for the task body. I think we should keep it, as it aligns with how the left sidebar and most lists in Nextcloud behave.

@raimund-schluessler
Copy link
Member Author

I will have a look at the issues I mentioned tomorrow, but feel free to work on it in the meantime.

@timkrief
Copy link
Member

@raimund-schluessler Thanks for the in-depth feedback. I did not want to touch to the structure / Vue components too much, that's why I used the subtasks-container to kind of hide the round corners when necessary. If we could have classes for each case you described, then we would have a cleaner solution for sure.

You removed the background-color on hover and active for the task body. I think we should keep it, as it aligns with how the left sidebar and most lists in Nextcloud behave.

Is that something new? I'm using Nextcloud 16 I don't have the background changing on hover in lists. Not even in the vanilla tasks app. I am not sure we should add that on tasks personally, but that's another concern.

@raimund-schluessler
Copy link
Member Author

I did not want to touch to the structure / Vue components too much, that's why I used the subtasks-container to kind of hide the round corners when necessary. If we could have classes for each case you described, then we would have a cleaner solution for sure.

I think that would be good.

Is that something new? I'm using Nextcloud 16 I don't have the background changing on hover in lists. Not even in the vanilla tasks app. I am not sure we should add that on tasks personally, but that's another concern.

That's how current master looks on Nextcloud 19:
grafik

Nextcloud 16 is end-of-life since 2020-06-04, would be good to update anyway 😉

@timkrief
Copy link
Member

Okay thanks, time pass by really fast right? I'll have to update my instance then so that I can work on up to date software. Sorry for the inconvenience.

@raimund-schluessler
Copy link
Member Author

Okay thanks, time pass by really fast right? I'll have to update my instance then so that I can work on up to date software. Sorry for the inconvenience.

No need to apologize. It's rare enough that someone actually contributes code 👍

@raimund-schluessler
Copy link
Member Author

Memo to myself: CSS selector to show a round corner at the top left for tasks whose previous sibling has subtasks shown:

li.task-item.hasSubtasksVisible + li.task-item > div.task-body {
    border-top-left-radius: var(--border-radius-large);
}

@timkrief
Copy link
Member

@raimund-schluessler I think you should also select when user is adding a subtask to the previous task. Something like

li.task-item.hasSubtasksVisible + li.task-item > div.task-body,
li.task-item.hasAddSubtaskVisible + li.task-item > div.task-body {
   border-top-left-radius: var(--border-radius-large);
}

Because a task could have no subtasks or all subtasks hidden but have the adding subtask input displayed.

timkrief and others added 7 commits July 23, 2020 20:22
Here's an updated style.scss that matches the designs I shown in issue #664

Signed-off-by: Tim Krief <[email protected]>
The lines between two rows on the same level were rounded off on the left, which looked a bit off. Now they are straight.

Signed-off-by: Tim Krief <[email protected]>
Added a subtasksHidden class when all class are hidden in a task-item and fixed the style for tasks with hidden subtasks.

Signed-off-by: Tim Krief <[email protected]>
Fixed an edge case where the style would not work for the last element of the tree with all subtasks hidden and the input field for adding subtasks displayed.

Signed-off-by: Tim Krief <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Member Author

I fixed all issues, and I think it looks really good, see #1136 (comment).

@timkrief
Copy link
Member

Congrats! You achieved that pretty quickly. I'm setting up a local dev environment so that I can properly try it, but just by the look of your screenshots, it looks like what I had, and even better with your fixes. I'm really hyped about this!

@raimund-schluessler
Copy link
Member Author

The only issue is that while dragging a task, the round corners are not correctly updated, since the new subtask relationship only really changes after the task was dropped. However, with var(--border-radius) it is barely even noticeable and probably not worth the hassle to add and remove the correct classes manually while dragging:

dragdrop

Also, I created a build, so anyone can test it easily:
tasks.tar.gz
And if no one objects, I would like to merge the PR soon.

@timkrief
Copy link
Member

Nice job! I just tried it and it feels really nice. Here are the four little details that I found while trying out the build.

  • I have a hard time finding why but when I'm trying to add a subtask to the last task of the tree the border radius is not right:

image

  • It would be nice when in "mobile mode" (narrow screens/windows) for the top level tasks' corners to be flat since they are against the borders of the window:

image
(sorry for the bad test task names ^^' )

  • The checkbox with no priority, it's using the default behavior of an usual checkbox which is to turn blue when hovered. I think it should be avoided here since it could create confusion with the meaning a colored checkbox now has in this case (priority). (Plus since it's now using 2px borders I think a lighter grey could be a better fit, but that could be subjective.)
    peek

  • The last thing is just the check-boxes not being centered (and that might be partly my fault). I think padding: 11px 10px; instead of padding: 12px 10px; should work fine.
    image

👍

@raimund-schluessler
Copy link
Member Author

@timkrief Thanks for the feedback, I will take care of the issues. 👍

Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Member Author

I fixed all issues you found:

tasklist

The task checkbox color might be a bit to light now, but it's the Nextcloud default border color var(--color-border-dark) and I did not want to introduce another color.

@jancborchardt
Copy link
Member

Btw, only one design detail I noticed with the design improvements from here: The shadow currently is blurring into every direction equally. Usually 1px shift to the bottom makes it look nicer (ref sun and real world and such ;)

(In the pull request it was also shifted 1px to bottom and right at some point, but that looks strange as well. Only shifting to bottom looks best.)

@raimund-schluessler
Copy link
Member Author

Btw, only one design detail I noticed with the design improvements from here: The shadow currently is blurring into every direction equally. Usually 1px shift to the bottom makes it look nicer (ref sun and real world and such ;)

(In the pull request it was also shifted 1px to bottom and right at some point, but that looks strange as well. Only shifting to bottom looks best.)

I purposely didn't do that, because I didn't want to introduce such a 3D effect. I think the general interface design of the last years went away from 3D effects to a more flat appearance.

@grandeljay
Copy link

grandeljay commented Aug 10, 2020

Wow I really like the new changes, they look really good. But I feel like they do not fit in with the rest of Nextcloud. I'm not hoping these changes to be undone but rather that NC move more into this direction, is anything like this planned in the future?

Edit:
In particular I think the grey background is very unlike NC

Sorry if I'm wrong here, kindly point me to the correct thread please.

@stefan-niedermann
Copy link
Member

stefan-niedermann commented Aug 10, 2020

@grandeljay i can see at least a PR for changing the background of the Deck app. So if you are interested into pushing this topic forward, i'd suggest to have a look at the other apps and make fitting suggestions which elements can be adapted - i don't think that those changes qualifies as a "theme" which simply can be generously stamped at each app 🤷‍♂

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants