Skip to content

Conversation

@timkrief
Copy link
Member

Here's an updated style.scss that matches the designs I shown in issue #664

The task list looks like this for light mode
image
and like this for dark mode
image

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
+ Coverage   25.56%   27.82%   +2.26%     
==========================================
  Files          40       48       +8     
  Lines        2496     2609     +113     
  Branches      494      494              
==========================================
+ Hits          638      726      +88     
- Misses       1718     1743      +25     
  Partials      140      140              

@jancborchardt
Copy link
Member

Looks very nice @timkrief, only 1 feedback:
The lines between 2 rows on the same level are rounded off on the left, which looks a bit off. I’d say it looks better if they are just straight?

Also you are now invited to the Nextcloud organization on Github, so in the future you don’t need your fork anymore and can just open branches in the main repository for better collaboration (don’t worry, the master branch is protected so nothing can break). :)

@timkrief
Copy link
Member Author

Thanks for the feedback @jancborchardt I fixed the lines in a new commit 7d5fb07 . It looks way better now.
image

@timkrief
Copy link
Member Author

Wait I still have to fix an issue when subtasks are hidden. I thought the structure for a task with no subtasks and a tasks with hidden subtasks was the same but it is not.

@raimund-schluessler
Copy link
Member

Wait I still have to fix an issue when subtasks are hidden. I thought the structure for a task with no subtasks and a tasks with hidden subtasks was the same but it is not.

Let me know if you need any help. I can have a look in the evening if necessary.

@timkrief
Copy link
Member Author

@raimund-schluessler I'm adding a subtasksHidden class to the task-item, when !showSubtasks in TaskBody.vue, I think that will be enough to be able to select it and stylize it with CSS without touching to the structure which makes sense as it is.

@timkrief
Copy link
Member Author

Ok, I added the class which let me stylize that element as needed, tasks with hidden subtasks now work. Let me know if there's anything else.

@raimund-schluessler
Copy link
Member

I will have a look in the evening. But since there are quite some changes here, you will have to signoff your commit(s), see https://github.com/nextcloud/tasks/pull/1135/checks?check_run_id=894171552.

timkrief added 3 commits July 21, 2020 14:49
Here's an updated style.scss that matches the designs I shown in issue nextcloud#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]>
timkrief added 2 commits July 21, 2020 15:00
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]>
@timkrief
Copy link
Member Author

timkrief commented Jul 21, 2020

Sorry for the hiccups, it's my first time contributing to nextcloud. I signed off the commits, removed some of my CSS I wasn't using anymore and fixed an edge-case I forgot of a task with the adding subtasks input displayed but subtasks hidden at the end of the tree.

@raimund-schluessler
Copy link
Member

Sorry for the hiccups, it's my first time contributing to nextcloud. I signed off the commits, removed some of my CSS I wasn't using anymore and fixed an edge-case i forgot of a task with the adding subtasks input displayed but subtasks hidden at the end of the tree.

Don't worry, take your time. I won't have time to look at it before the evening anyway.

@raimund-schluessler
Copy link
Member

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

I took the liberty to push your branch to the Tasks repository and create a new PR in #1136 so we can better collaborate on the PR (it's a bit cumbersome to push to someone elses branch, even if its allowed).

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