Feature #15361
closed
Use css pseudo-classes instead of cycle("odd", "even")
0%
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?
Files
Updated by Jean-Philippe Lang over 10 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.
Updated by Jean-Philippe Lang over 10 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
Updated by Marius BĂLTEANU almost 8 years ago
- File use_pseudo_classes.patch use_pseudo_classes.patch added
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:- 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).
- 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.
- For drag'n'drop feature (Redmine 3.3.0 #12909) it is no longer required to recalculate the odd/even classes using JS.
Updated by Go MAEDA almost 8 years 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.
Updated by Go MAEDA almost 8 years ago
- Target version changed from Candidate for next major release to 3.4.0
Updated by Marius BĂLTEANU over 7 years ago
Updated the patch to cleanly apply on the current trunk and also to include the new "cycle("odd", "even")" added in r15466
Updated by Jean-Philippe Lang over 7 years 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.
Updated by Marius BĂLTEANU over 7 years 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.
Updated by Jean-Philippe Lang over 7 years 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?
Updated by Jean-Philippe Lang over 7 years ago
FTR, I've reverted the changes until we find a solution for note-7 and take the decision to integrate the change.
Updated by Marius BĂLTEANU over 7 years ago
- File issues_list.png issues_list.png added
- File 3.4.0_use_pseudo_classes_v3.patch 3.4.0_use_pseudo_classes_v3.patch added
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.
Updated by Marius BĂLTEANU over 7 years 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.
Updated by Jean-Philippe Lang about 7 years 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.
Updated by Marius BĂLTEANU about 7 years 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.
Updated by Marius BĂLTEANU about 7 years ago
- File add_odd-even_class_to_time_entries_block_from_my_page.patch add_odd-even_class_to_time_entries_block_from_my_page.patch added
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.
Updated by Go MAEDA about 7 years ago
- Status changed from Closed to Reopened
Reopening to handle #15361#note-15.
Updated by Jean-Philippe Lang about 7 years ago
- Status changed from Reopened to Closed
Patch applied, thanks.