Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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"
Comment | File | Size | Author |
---|---|---|---|
#49 | views.code_.1360186-49.patch | 3.34 KB | Pol |
#47 | views.code_.1360186-47.patch | 3.34 KB | Pol |
#46 | views.code_.1360186-46.patch | 1.26 KB | Pol |
#45 | views.code_.1360186-44.patch | 4.74 KB | Pol |
#41 | views.code_.1360186-41.patch | 4.72 KB | Pol |
Comments
Comment #1
davidwhthomas CreditAttribution: davidwhthomas commentedI'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
Comment #2
rokr CreditAttribution: rokr commentedI'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?
Comment #3
davidwhthomas CreditAttribution: davidwhthomas commented@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.
Comment #4
rokr CreditAttribution: rokr commentedDavid, 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
Comment #5
mikeprinsloo CreditAttribution: mikeprinsloo commentedI can also confirm that the patch applies cleanly and works as expected with the current dev release.
Comment #6
rokr CreditAttribution: rokr commentedComment #7
merlinofchaos CreditAttribution: merlinofchaos commentedI agree with the RTBC.
Comment #8
dawehnerWoW 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.
Comment #9
dawehnerOkay i reverted this patch as it caused a lot of troubles #1448400: Row classes not showing since update
Comment #10
davidwhthomas CreditAttribution: davidwhthomas commentedHmm sorry to hear that, I wonder what needs adjusting?
Has been working fine from this corner..
update: am tracking the related issue.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented3.2 release was working fine here too and I wasn't seeing any issues reported in #1448400: Row classes not showing since update.
Comment #12
eduardogz CreditAttribution: eduardogz commentedI 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 ;)
Comment #13
Norberto Ostallo CreditAttribution: Norberto Ostallo commentedAny 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.
Comment #14
scitoIt'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]
.Comment #15
davidwhthomas CreditAttribution: davidwhthomas commentedI can see why the issue with
becoming
occured as per http://drupal.org/node/1448400#comment-5635180
and that can be fixed by adding a space between the classes
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.
Comment #16
davidwhthomas CreditAttribution: davidwhthomas commentedOK, 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
Comment #17
aasarava CreditAttribution: aasarava commentedPatch in #16 works for me. Thank you!
Comment #18
jaroslaw.kaminski CreditAttribution: jaroslaw.kaminski commentedwhat about this task? I see that this patch is not added into views module. Why?
Comment #19
dawehnerCouple of validations against drupal code style: some space, missing new line for else ...
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.
Comment #20
jaroslaw.kaminski CreditAttribution: jaroslaw.kaminski commentedI 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.
Comment #21
jaroslaw.kaminski CreditAttribution: jaroslaw.kaminski commentedOk, 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 ;-)
Comment #23
jaroslaw.kaminski CreditAttribution: jaroslaw.kaminski commentedhah, 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 :(
Comment #24
GiorgosKhere 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
Comment #25
dawehner@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.
Comment #26
jaroslaw.kaminski CreditAttribution: jaroslaw.kaminski commentedis 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.
Comment #27
Mołot CreditAttribution: Mołot commentedSending last patch to testbot.
Comment #29
dawehnerWell, 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.
Comment #30
davidwhthomas CreditAttribution: davidwhthomas commentedHere'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));
Comment #32
Mołot CreditAttribution: Mołot commentedYour 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.
Comment #33
codi CreditAttribution: codi commentedHoping this will pass the tests. Fingers crossed.
Comment #34
codi CreditAttribution: codi commentedTestbot superman jenga dragon-time.
Comment #35
drwits CreditAttribution: drwits commentedconfirming patch in #33 works
thanks!
Comment #36
Yoran Scholiers CreditAttribution: Yoran Scholiers commentedLooks great, I hope this get into the module fast :)
Comment #37
PolAlso tested and working perfectly. Can't wait to see this in.
PS: The patch also apply on stable version 7.x-3.5.
Comment #38
PolNew patch including class cleaning for "Style settings" in the field.
Comment #39
PolNew patch, I forgot to replace some 'element_classes' and 'element_label_classes' methods.
We are using this patch already on our production site.
Comment #40
dawehnerPlease 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.
Comment #41
PolHere it is...
Comment #43
Pol#41: views.code_.1360186-41.patch queued for re-testing.
Comment #45
PolFix patch.
Comment #46
PolHere'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.Comment #47
PolNew 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.
Comment #48
dawehnerI prefer to use count() as it is more explicit.
Comment #49
PolNew patch using count() instead of sizeof().
Comment #50
PolAny update on this please ?
Comment #51
PolAnybody home ? :)
Comment #52
Mołot CreditAttribution: Mołot commentedWorks for me, if that matters
Comment #53
dgtlmoon CreditAttribution: dgtlmoon commentedI'm not sure about this approach, it does sort of solve the problem but it doesn't actually fix the cause of the problem
Comment #54
PolLooks like nobody cares about this issue, so, we need to find a way to make it work properly !
Comment #55
dgtlmoon CreditAttribution: dgtlmoon commented@Pol what do you think about my patch https://www.drupal.org/node/1262630#comment-9223845 , Can you test?
Comment #56
Yuri CreditAttribution: Yuri commentedI have installed the patch #49 but it does nothing in my view (view 7.x-3.10).
Comment #57
PolI will provide an updated patch next week.
However, you should see new tokens in the list.
Comment #58
AndrackOSnack CreditAttribution: AndrackOSnack commentedHi,
any update on this ? There is also the patch #24 in #1262630 that works well..
Comment #60
jacquelynfisher CreditAttribution: jacquelynfisher commentedIs 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?
Comment #61
RAWDESK CreditAttribution: RAWDESK commentedWithout 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 :)
Comment #62
Jessica.K CreditAttribution: Jessica.K as a volunteer commentedRan 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.