Feature #15361

Use css pseudo-classes instead of cycle("odd", "even")

Added by Vadim Pushtaev almost 4 years ago. Updated 7 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Code cleanup/refactoring
Target version:3.4.0
Resolution:Fixed

Description

I'm not sure it's a right way to ask such questions, so I'm sorry in advance.

I'm trying to produce some solution for making drag'n'drop in tables possible (see #12909).

I have a problem with class="<%= cycle("odd", "even") %>">, static classes are not suitable for drag'n'drop.

What do you think about using :odd and :even css pseudo-classes just to avoid static classes? I can make a patch, but will it be accepted?

use_pseudo_classes.patch Magnifier (27.6 KB) Marius BALTEANU, 2016-05-31 00:47

use_pseudo_classes_v2.patch Magnifier (28.3 KB) Marius BALTEANU, 2016-08-23 00:26

issues_list.png (81.2 KB) Marius BALTEANU, 2016-12-05 23:57

3.4.0_use_pseudo_classes_v3.patch Magnifier (29.4 KB) Marius BALTEANU, 2016-12-06 00:07

add_odd-even_class_to_time_entries_block_from_my_page.patch Magnifier (1.38 KB) Marius BALTEANU, 2017-01-25 20:53

Associated revisions

Revision 16050
Added by Jean-Philippe Lang 9 months ago

Use css pseudo-classes instead "odd", "even" classes (#15361).

Patch by Marius BALTEANU.

Revision 16051
Added by Jean-Philippe Lang 9 months ago

Removes calls to #reset_cycle (#15361).

Revision 16052
Added by Jean-Philippe Lang 9 months ago

Reverts r16051 and r16050 for now (#15361).

Revision 16249
Added by Jean-Philippe Lang 7 months ago

Use css pseudo-classes instead of cycle("odd", "even") (#15361).

Patch by Marius BALTEANU.

Revision 16250
Added by Jean-Philippe Lang 7 months ago

Fixes row background for alternate theme (#15361).

Revision 16259
Added by Jean-Philippe Lang 7 months ago

Remove background on subtasks and relations introduced in r16249 (#15361).

Revision 16262
Added by Jean-Philippe Lang 7 months ago

Adds odd_even class to time entries (#15361).

Patch by Marius BALTEANU.

History

#1 Updated by Jean-Philippe Lang almost 4 years ago

You should be able to reassign the appropriate css class to each row after drag'n'drop easily using javascript. But using pseudo-classes seems an interesting option to clean up the views.

#2 Updated by Jean-Philippe Lang almost 4 years ago

  • Tracker changed from Defect to Feature
  • Subject changed from cycle("odd", "even") instead of css pseudo-classes to Use css pseudo-classes instead of cycle("odd", "even")
  • Category set to Code cleanup/refactoring

#3 Updated by Marius BALTEANU about 1 year ago

I've created a patch that replaces the cycle("odd", "even") with CSS pseudo-classes. The patch passes the tests.

Some remarks regarding this patch:
  1. In current version, roadmap and version pages don't have the odd/even classes, but with this patch will have the styles (which is good for consistency in my opinion).
  2. To keep the current style of block "Spent Time" from My Page (total row with "odd" style and time entries with white background), it was required to add two new CSS rules to specifically target that rows.
  3. For drag'n'drop feature (Redmine 3.3.0 #12909) it is no longer required to recalculate the odd/even classes using JS.

#4 Updated by Go MAEDA about 1 year ago

  • Target version set to Candidate for next major release

Although the patch may break some themes, it makes view files cleaner and can save some processor cycles on web servers.

#5 Updated by Go MAEDA about 1 year ago

  • Target version changed from Candidate for next major release to 3.4.0

#6 Updated by Marius BALTEANU about 1 year ago

Updated the patch to cleanly apply on the current trunk and also to include the new "cycle("odd", "even")" added in r15466

#7 Updated by Jean-Philippe Lang 9 months ago

I've committed the patch but it comes with an undesirable change: when displaying issue descriptions on the issue list, we used to get the issue row and its description with the same background color. Now, the issue and the description have their own background with different colors. It kind of breaks the readability of the list. Not sure if we can fix it using CSS only.

#8 Updated by Marius BALTEANU 9 months ago

Jean-Philippe Lang wrote:

I've committed the patch but it comes with an undesirable change: when displaying issue descriptions on the issue list, we used to get the issue row and its description with the same background color. Now, the issue and the description have their own background with different colors. It kind of breaks the readability of the list. Not sure if we can fix it using CSS only.

I'll come up with a solution to fix this issue.

#9 Updated by Jean-Philippe Lang 9 months ago

Themes that colorize issues based on odd/even classes will have to get fixed (including the "alternate" theme provided with the core). Is the removal of 2 javascript lines worth this pain? I mean, what is the real benefit of using the CSS pseudo classes?

#10 Updated by Jean-Philippe Lang 9 months ago

FTR, I've reverted the changes until we find a solution for note-7 and take the decision to integrate the change.

#11 Updated by Marius BALTEANU 9 months ago

I've attached a new patch that fixes the issue from note-7. The patch includes both revisions (r16050 and r16051).

The simplest solution found was to keep the odd/even classes in the "issues/_list.html.erb" and "timelogs/_list.html.erb" views. The fix required also to have a new class named "odd-even" for the tables that still use the odd/even classes.

#12 Updated by Marius BALTEANU 9 months ago

Jean-Philippe Lang wrote:

Themes that colorize issues based on odd/even classes will have to get fixed (including the "alternate" theme provided with the core). Is the removal of 2 javascript lines worth this pain? I mean, what is the real benefit of using the CSS pseudo classes?

The new patch solves also the "alternate" theme. For other themes, the required fix is very small.

Regarding the real benefits, from my point of view:
- cleaner view files
- using a native and client-side only solution instead one that requires server-side processing and client-side
- simple way to style the tables (if you have a simple table, you only need the "list" class on the table; if you have a more complex table and you want to control the odd and even rows, you need the "list odd-even" classes and the "cycle" implementation).

For us, this feature is not a big improvement because in our custom theme we use the same color for rows and a bottom border as delimiter.

#13 Updated by Jean-Philippe Lang 7 months ago

  • Status changed from New to Closed
  • Assignee set to Jean-Philippe Lang
  • Resolution set to Fixed

Patch is committed, thanks Marius. I still had to fix the backgrounds for the alternate theme.

#14 Updated by Marius BALTEANU 7 months ago

Jean-Philippe Lang wrote:

Patch is committed, thanks Marius. I still had to fix the backgrounds for the alternate theme.

I'm sure that I looked into the problem. I'll apply again the patch without your fix to understand what was the problem. Thanks.

#15 Updated by Marius BALTEANU 7 months ago

I still can't figure it out which was the problem with the alternate theme, I miss something, for sure. But is not so important.

r16249 adds the :not(.odd-even) to the respective selectors and make them more powerfull than the rule "div.mypage-box table.time-entries tr.time-entry". The attached patch fixes the issue generated by that change and keeps the current style of the block "Spent Time" from My Page.

#16 Updated by Go MAEDA 7 months ago

  • Status changed from Closed to Reopened

Reopening to handle #15361#note-15.

#17 Updated by Jean-Philippe Lang 7 months ago

  • Status changed from Reopened to Closed

Patch applied, thanks.

Also available in: Atom PDF