It's great that we can use a field to set the row class of a view display but it is literally putting the raw text in without making it CSS friendly.

e.g. I am using the term field to set the row class where my terms are "TTC Event", "TTC Performance", "External". These end up exactly like this in the generated CSS (with spaces...) rather than being converted to "ttc-event", "ttc-performance" and "external"

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidwhthomas’s picture

Status: Active » Needs review
FileSize
1.06 KB

I've had the same issue.

Adding a row class wasn't properly escaped with the resulting class containing spaces

e.g class="directory-Special Node my author"

The attached patch fixes the issue, for multiple and single row classes.

e.g, classes become class="directory-Special-Node my-author"

Note, you can test by adding a row class to the format settings, e.g Format:Unformatted list | Settings

Please review and commit when possible.

thanks,

DT

rokr’s picture

Version: 7.x-3.0-rc3 » 7.x-3.x-dev
Category: bug » feature

I'm not sure if this is a task, views should resolve.
You can do a lot of rewriting at field level and i think you should know what you are doing and - in that case - rewrite fields to your needs.

What do you think?

davidwhthomas’s picture

Category: feature » bug

@rokr, this is a bug fix for an existing feature in views.

The ability to "Add row class" is already in the style settings form, but it's broken.

This patch simply fixes the error to actually sanitize the string for output as one or more valid CSS classes.

Reassigning to bug report on that basis.

rokr’s picture

David, i checked that out. Thanks for correcting me.
I applied your patch and it works as expected for e.g. taxonomy terms consisting of two words.

Thank you, cheers, Ronald

mikeprinsloo’s picture

I can also confirm that the patch applies cleanly and works as expected with the current dev release.

rokr’s picture

Status: Needs review » Reviewed & tested by the community
merlinofchaos’s picture

I agree with the RTBC.

dawehner’s picture

Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

WoW a RTBC by multiple people, that's really uncommon :)
Thanks for providing the patch!

Committed to 7.x-3.x, though the port to 6.x-3.x need some work.

dawehner’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Patch (to be ported) » Needs work

Okay i reverted this patch as it caused a lot of troubles #1448400: Row classes not showing since update

davidwhthomas’s picture

Hmm sorry to hear that, I wonder what needs adjusting?

Has been working fine from this corner..

update: am tracking the related issue.

Anonymous’s picture

3.2 release was working fine here too and I wasn't seeing any issues reported in #1448400: Row classes not showing since update.

eduardogz’s picture

I would just like to point out that transliteration should also be brought into the css-friendly string sanitation process.

I ran into this when trying to assign a css class based on a taxonomy term in Spanish, the character 'é' went right in to the css class, untouched, unharmed ;)

Norberto Ostallo’s picture

Any update on this issue?
I have tested the patch in #1 and it seems to fix row class issue correctly, without any of the problems described in #1448400: Row classes not showing since update.

scito’s picture

It's a pity that this patch causes side-effects. The approach of this patch seems the right one to me.

Is there a possibility to add this patch again? Maybe in a two stage procedure? Deprecate the old behavior of automatically adding spaces, e.g. in between of text[tag].

In a second stage, a release later, recommit this patch again. Maybe we could trim whitespace in order to address the rationale behind the current usage of text[tag].

davidwhthomas’s picture

I can see why the issue with

clearfix[tl_main_contact]

becoming

clearfix-main-contact

occured as per http://drupal.org/node/1448400#comment-5635180

and that can be fixed by adding a space between the classes

clearfix [tl_main_contact]

http://drupal.org/node/1448400#comment-5636372
http://drupal.org/node/1448400#comment-5637808

But I'm not sure why no row class would show at all, as I'm unable to replicate the problem with no row class showing.

Once the cause of that issue is identified, I can roll a new patch.

DT

P.S Update: I have a theory, new patch pending.

davidwhthomas’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

OK, I see what the issue was there.

The flaw in the patch was that it was only applying the row class for view displays that used fields.

if ($this->uses_fields() && $this->view->field) { ...

So users with 'content' type view displays would have lost their row classes.

The attached updated patch fixes the issue and applies to both field and normal 'content' style view displays.

Token substitutions are applied if the view display uses fields.

As per eduardogz's request in #14, it also applies transliteration to the classes, if the transliteration module is enabled.

Please review and commit when possible.

Thanks!

DT

aasarava’s picture

Patch in #16 works for me. Thank you!

jaroslaw.kaminski’s picture

what about this task? I see that this patch is not added into views module. Why?

dawehner’s picture

+++ b/plugins/views_plugin_style.incundefined
@@ -122,16 +122,24 @@ class views_plugin_style extends views_plugin {
+        }else{
...
+          }          ¶
...
+        return implode(' ', $classes);

Couple of validations against drupal code style: some space, missing new line for else ...

+++ b/plugins/views_plugin_style.incundefined
@@ -122,16 +122,24 @@ class views_plugin_style extends views_plugin {
+        if(module_exists('transliteration')){
+          $classes = array_map('transliteration_get', $classes);
+        }

Views really tries to not use other contrib module, can't we totally avoid that?

In general patches aren't just committed, but we need people to test it, people should have reviewed the actual code etc.

jaroslaw.kaminski’s picture

In general patches aren't just committed, but we need people to test it, people should have reviewed the actual code etc.

I understand you, and I'm know that in Drupal 8 it will be harder to put new patches into stable tree, but I think this patch is important and it would be great if it will be commited into stable tree.

I'm preparing new patch, cause author forgot about strtolower in class name.

jaroslaw.kaminski’s picture

Ok, I made a new patch with strtolower in class name. I hope that drupal code style is proper ( I use phpstorm with drupal addons ), if not I want to apologise for that ;-)

Status: Needs review » Needs work

The last submitted patch, views-add-strtolower-1360186-21.patch, failed testing.

jaroslaw.kaminski’s picture

hah, funny thing, I've only added strtolower in class name ( and remove white space ). I see that QA systems are much more better than almost year ago ;-p

Can anyone prepare better patch? I dont have time to make another one :(

GiorgosK’s picture

FileSize
1.38 KB

here is another take on patch #21
but I am not really good with patches (it would be a miracle if it passed formatting tests)

I say RTBC, it works as advertised

dawehner’s picture

@jaroslaw.kaminski
Well the problem is that views hasn't been converted to the new codestyle, because there have been more important things todo.

... this @transliteration snippet should probably still be removed.

jaroslaw.kaminski’s picture

is there any chance that one of our latest patch ( GiorgoSK or mine ) will be added into views module in near future? I'm always afraid to use patches, cause its big possibility that me or my collegues will forget about patch module in every update ;-)

@transliteration. By this document: http://www.w3.org/TR/CSS21/syndata.html#characters we can use nationals signs as class names ftp://www.columbia.edu/kermit/public_html/utf8-t1.html so we can remove transliteration snippet but in my opinion it would be not comfortable to use non converted names ;-) So make a decision please and I can create next patch without transliteration.

Mołot’s picture

Status: Needs work » Needs review

Sending last patch to testbot.

Status: Needs review » Needs work

The last submitted patch, 1360186-24.patch, failed testing.

dawehner’s picture

@transliteration. By this document: http://www.w3.org/TR/CSS21/syndata.html#characters we can use nationals signs as class names ftp://www.columbia.edu/kermit/public_html/utf8-t1.html so we can remove transliteration snippet but in my opinion it would be not comfortable to use non converted names ;-) So make a decision please and I can create next patch without transliteration.

Well, if the admin things to put any kind of characters in there, it's not the problem of views to take care of it, IMHO.

davidwhthomas’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

Here's an updated version of my original patch from #16 that works with both field and content based view displays.

It also includes drupal_html_class which calls drupal_clean_css_identifier(drupal_strtolower($class));

Status: Needs review » Needs work

The last submitted patch, 1360186-views-row-class-cleansing-2.patch, failed testing.

Mołot’s picture

Your patch failed testing on one test only:
"Take sure that a custom css class is added to the output."

Seems about right. Could you explain what changes it makes to css? And maybe include needed test changes to reflect that? Should be relatively easy, just apply the same transformation to what tests look for.

codi’s picture

Hoping this will pass the tests. Fingers crossed.

codi’s picture

Status: Needs work » Needs review

Testbot superman jenga dragon-time.

drwits’s picture

confirming patch in #33 works

thanks!

Yoran Scholiers’s picture

Looks great, I hope this get into the module fast :)

Pol’s picture

Status: Needs review » Reviewed & tested by the community

Also tested and working perfectly. Can't wait to see this in.

PS: The patch also apply on stable version 7.x-3.5.

Pol’s picture

New patch including class cleaning for "Style settings" in the field.

Pol’s picture

New patch, I forgot to replace some 'element_classes' and 'element_label_classes' methods.

We are using this patch already on our production site.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Please don't change patches dramatically and leave it RTBC.

In general i don't think we should remove functions given that especially this functionality could be indeed used by people in contrib/custom development.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.72 KB

Here it is...

Status: Needs review » Needs work

The last submitted patch, views.code_.1360186-41.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review

#41: views.code_.1360186-41.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, views.code_.1360186-41.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
4.74 KB

Fix patch.

Pol’s picture

Here's a new solution fixing the two previous problems.

I'm just providing new tokens, transliterated and you can used them where you want.

The only thing is that it's using the module_exists() function to check if the 'transliteration' module exists.

Pol’s picture

New patch using a hook as suggested by merlinofchaos on IRC.

The implementation of the hooks are in Views just to show how it works, it could be in another module of course.

dawehner’s picture

+++ b/handlers/views_handler_field.inc
@@ -315,6 +315,12 @@ class views_handler_field extends views_handler {
+        if (sizeof(module_implements('views_field_token_alter')) > 0) {

@@ -796,6 +802,13 @@ class views_handler_field extends views_handler {
+      // Make sure at least one module implements our hook.
+      if (sizeof(module_implements('views_field_token_alter_info')) > 0) {

@@ -1422,6 +1435,12 @@ If you would like to have the characters \'[\' and \']\' please use the html ent
+    if (sizeof(module_implements('views_field_token_alter')) > 0) {

I prefer to use count() as it is more explicit.

Pol’s picture

New patch using count() instead of sizeof().

Pol’s picture

Any update on this please ?

Pol’s picture

Issue summary: View changes

Anybody home ? :)

Mołot’s picture

Works for me, if that matters

dgtlmoon’s picture

I'm not sure about this approach, it does sort of solve the problem but it doesn't actually fix the cause of the problem

Pol’s picture

Looks like nobody cares about this issue, so, we need to find a way to make it work properly !

dgtlmoon’s picture

@Pol what do you think about my patch https://www.drupal.org/node/1262630#comment-9223845 , Can you test?

Yuri’s picture

I have installed the patch #49 but it does nothing in my view (view 7.x-3.10).

Pol’s picture

I will provide an updated patch next week.
However, you should see new tokens in the list.

AndrackOSnack’s picture

Hi,

any update on this ? There is also the patch #24 in #1262630 that works well..

jacquelynfisher’s picture

Is there a patch in this thread that is working for this?

I, too, need to create a row class using a token from a field, but the token is pulling in spaces (not CSS friendly).

For example, it is outputting: <tr class="My Field Name">

when the spaces need to be converted to dashes, like this: <tr class="My-Field-Name"> or <tr class="my-field-name">

Help?

RAWDESK’s picture

Without applying any of the above patches, i've found a workaround by
- adding a relationship to the taxonomy term vocabulary
- add Taxonomy TID as (excluded from display) field
- use [tid] as token replacement in the row class setting
Guess that won't cause any 'spacial' problems :)

Jessica.K’s picture

Status: Needs review » Needs work

Ran into this issue today and really need it to work. All of these patches are rolled against older versions and had inaccuracies/out of date placement so I had to pick through applying by hand.

I initially tried the patch in #49, which did nothing. All token classes were still applying with spaces and capitals after clearing all caches just to be certain. There were no new tokens available that were supposed to be transliterated like mentioned in subsequent comments. Reverted to Views 7.x-3.20 to remove the patch.

Scrolled back to the most recent patch that multiple people agreed was working, which is #33. It works, no errors or other undesired behavior in Views.

I'm not sure why the method in #33 was abandoned (no clear justification was given on the patches supplied after), but it should probably be revisited for the current version.