Updated: Comment #19
Problem/Motivation
The HEX codes in the color module fields display the # at the end of the numbers in RTL languages, and they should still be shown at the front. @neo1691 reproduced the bug by enabling language module, changing language to RTL, and then editing theme color settings.
Proposed resolution
Correct the CSS in color.admin.css - which will set the direction every time the color changes.
Changing via js was also proposed but CSS approach preferred.
Patch at #17 by aparnakondala123 has been reviewed by Mark Carver, mradcliffe & kattekrab.
Patch at #25 appears to have resolved the issue by replacing the css selector with an #attributes array.
Before applying patch #25:
After applying patch #25:
Remaining tasks
Possible Follow-ups
1. bug report should be posted regarding issue in #1 "(When I clicked settings, I got a page not found message, workaround was to disable the overlay module and enable it again, maybe another bug)"
2. Some strings stand out in manual test screenshot as not translated.
Original report by djbobbydrake
The RTL hexadecimal color code formatting is incorrect. The "#" should always be on the left, even in RTL. Image below:
Comment | File | Size | Author |
---|---|---|---|
#26 | before.png | 42.75 KB | Tim Bozeman |
#26 | after.png | 43.06 KB | Tim Bozeman |
#25 | 2003570-25-color-scheme-ltr.patch | 452 bytes | tstoeckler |
#19 | 2003570-rtl-hex-manualtest.png | 72.51 KB | kattekrab |
#17 | drupal8.color-module.2003570-17.patch.patch | 369 bytes | aparnakondala1 |
Comments
Comment #1
neo1691 CreditAttribution: neo1691 commentedI managed to reproduce this bug on drupal-8.0-alpha2
Enabled the language module in extend->multilingual->language
Changed the language to RTL
Configure->Languages->English->Edit->Direction->RTL
I went to Appearance->Bartik->Settings->Color Scheme
(When I clicked settings, I got a page not found message, workaround was to disable the overlay module and enable it again, maybe another bug)
Comment #2
vineet.osscube CreditAttribution: vineet.osscube commentedComment #3
vineet.osscube CreditAttribution: vineet.osscube commentedI have fixed this issue. I am attaching a patch for the same.
Comment #4
tstoecklerSorry for not trying this out, but is this markup produced somewhere without JS? Otherwise patch looks great, could use a comment, however.
Comment #5
markhalliwellThis bug should be fixed in color.admin.css, not in JS. This method sets the direction every time the color changes, we only need to do this once.
Comment #6
markhalliwellComment #7
aparnakondala1 CreditAttribution: aparnakondala1 commentedCreated a patch for thr RTL issue using CSS.
Comment #9
kattekrab CreditAttribution: kattekrab commented@aparnakondala123
Thanks for submitting a patch! :)
You will need to reroll this and correct the slashes and directory path. This is an easy mistake when using windows for development.
Make sure the path is relative to the drupal root.
Check the newline at end of file - something's wrong there too.
Comment #10
aparnakondala1 CreditAttribution: aparnakondala1 commentedPatch to solve this issue using CSS
Comment #11
aparnakondala1 CreditAttribution: aparnakondala1 commentedSomeone please review.
Comment #12
aparnakondala1 CreditAttribution: aparnakondala1 commentedSomeone please review.
Comment #13
markhalliwellThanks for the patch @aparnakondala123! I like the simpler approach. Here is a few minor coding standard nit picks:
The curly brackets should be on the same line per https://drupal.org/node/1887862#rule-sets. Also, read the "Styling for Right-To-Left Languages" section right above that.
I would also prefer to keep at least one class (or ID if no class exists) before the
input
portion to target just the fields in the color palette.Comment #14
aparnakondala1 CreditAttribution: aparnakondala1 commentedPatch to solve this issue using CSS
Thank you Mark Carver
Comment #15
mradcliffeIt looks like standards for CSS suggest that there must be a space after "{" and ":".
Good work so far.
Comment #16
markhalliwellShould be:
Comment #17
aparnakondala1 CreditAttribution: aparnakondala1 commentedUpdated patch
Comment #18
mradcliffePatch looks good, aparnakondala123!
Comment #19
kattekrab CreditAttribution: kattekrab commentedManual tested with simplytestme - seems to work!
Here's a screenshot.
removed "needs manual testing" tag.
Comment #19.0
kattekrab CreditAttribution: kattekrab commentedupdated issue summary.
Comment #20
kattekrab CreditAttribution: kattekrab commented-Needs issue summary update
Comment #21
markhalliwellThere needs to be a space after the comma (
,
).There needs to be a two space indention at the beginning, it's nested inside
{ }
.Comment #22
markhalliwellRemoved tag that was added back...
Comment #23
tstoecklerWouldn't another approach be to set the direction attribute directly as part of the form array in color_scheme_form()? I would find that more intuitive than via CSS. Don't want to rain on anyone's parade, though, the current approach is totally fine as well! Just wanted to ask if there are any reasons for either approach.
Comment #24
markhalliwellHmm, tbh either is fine really (ie: I have no preference). I just didn't like that it was doing CSS in JS.
Comment #25
tstoecklerHere's that approach. Hope it's OK for me to jump on this.
Tested this and get more or less the same screenshot as in #19.
Comment #26
Tim Bozeman CreditAttribution: Tim Bozeman commentedThis is a test of #25 before applying the patch:
and after:
Comment #26.0
Tim Bozeman CreditAttribution: Tim Bozeman commentedUpdated issue summary - link to latest comment
Comment #27
Tim Bozeman CreditAttribution: Tim Bozeman commentedLooks good to me!
#25++
Comment #28
Tim Bozeman CreditAttribution: Tim Bozeman commentedChanged issue title from "incorrect RTL hexadecimal color code format" to "Enabling RTL and then changing theme colors moves the hexadecimal # to the right."
Comment #29
alexpottCommitted 882c6f0 and pushed to 8.x. Thanks!
Comment #30.0
(not verified) CreditAttribution: commentedI made before and after pics of applying patch #25 and updated the proposed resolution.