A forgotten padding-right for a ul made inline links look wrong in RTL

From what I can tell, this is still an issue--however the patch needs to be re-rolled given that system.theme.css is gone (#2395853: Split system.module.css and system.theme.css files into SMACSS style components).

I'd like to work on this. I've never contributed and was looking for an easy win. Will try rerolling now.

I wanted to help, but I'm not really sure what work there is to complete. The patch is successful at the time it was created but has a conflict when attempting to rebase. As jhedstrom pointed out, the system.theme.css styles where relocated. In this case, the link styles were moved to core/themes/classy/css/components/links.css.

As far as I can tell, the updated stylesheet addresses this issue on line 13. Screenshot or rendered content attached. I'm not sure there is any work to be done, but would welcome a chance to get involved if someone knows better.

File system.theme.css is not longer in the core. So we have to add the same in classy or seven.

There was a issue with ltr & rtl mode both. Have done changes in the seven & classy theme.
See attached screenshot for reference.

#12 patch is apply success fully. It is working fine. I have attached before and after images.

Thanks for the work so far everyone.

+++ b/core/themes/classy/css/components/links.css
@@ -7,11 +7,13 @@ ul.inline,
+  margin-left: 0;

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,4 +38,5 @@ ul.inline li {
+  padding-left: 0;

I think both of these need an /* LTR */ comment and need to be undone in the RTL version. The padding-left one may need an equivalent RTL selector added if one doesn't exist.

https://www.drupal.org/node/1887862#rtl

@Cottser ,

Patch 8 and fixes both RTL and LTR , so therefore no RTL LTR comment necessary in this case.

Good to go.

@joseph.olstad I'm confused - it's a standard for a reason why wouldn't it apply here? I think there is also missing CSS not just comments…

1.


@Cottser I understand we have to add /* LTR */ in this. Because there is a styling exist for /* RTL */ as well

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,4 +38,5 @@ ul.inline li {
+  padding-left: 0;

but here, there is no other CSS properties is applied for /* RTL */ then why do we have to add /* LTR */ comment in this.

+++ b/core/themes/classy/css/components/links.css
@@ -7,11 +7,13 @@ ul.inline,
+  margin-left: 0;

2.

+++ b/core/themes/classy/css/components/links.css
@@ -7,11 +7,13 @@ ul.inline,
 [dir="rtl"] ul.inline,
 [dir="rtl"] ul.links.inline {
...
   padding-left: 15px;

@all Also, I am wondering. Why we need this padding for /* RTL */ ? Is it applied on some other pages ?

Added patch

Added interdiff file

Tested and verified on Ubuntu 14.04 for different browsers. Its working fine. Screenshots attached.

Uploaded patch for 8.3.x-dev branch. Please review

Assigning to @Cottser given prior involvement.

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,4 +38,9 @@ ul.inline li {
+  padding-left: 0; /* RTL */

An /* RTL */ comment isn't a thing. https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

For frontend changes like this before and after screenshots are very useful as well.

No need to place comments there, coz those styles are ok both, ltr and rtl

so if code is valid, then it is RTBC, screen is here

After this patch

Review the latest patch and this issue seems fixed now. So moving it to RTBC.

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,4 +38,9 @@ ul.inline li {
+  padding-left: 0; /* LTR */

Where is the corresponding RTL for this? What I see in the file in HEAD is:

ul.links li,
ul.inline li {
  padding-right: 1em; /* LTR */
}
[dir="rtl"] ul.links li,
[dir="rtl"] ul.inline li {
  padding-left: 1em;
  padding-right: 0;
}
ul.inline li {
  display: inline;
}

Also, is the menus and lists CSS file the right place to specify formatting for the comment add form? Seems unexpected to me.

Finally, this is introducing CSS changes in Classy, which is supposed to be stable. At the least, for that reason, the issue needs to be targeted for 8.4.x. The BC policy is:

We will change the markup and CSS that core generates for user interface and theme improvements. Specific core themes (Bartik, Seven) are considered internal and may change. The stable base themes (Stable, Classy) are considered public API, with stable templates, markup, and CSS, so themes needing BC support should extend one of those base themes.

Would the change to Classy comply with that? Tagging for subsystem maintainer review. Can we fix it just in module CSS instead? Is the comment module's own CSS perhaps a better place? Or, if the fix really belongs in Seven's menus and lists CSS, what other places on the site besides the comment form are affected?

I think when we test this we should add additional before-and-after screenshots of several different places where the CSS is used, both for RTL and LTR, pointing out why each part is necessary. The one in #29 is a good start but we need things to compare it to and it would help also to annotate it. The screenshots can be annotated to point out which each is and added to the summary. That will make this easier to review.

Thanks all for your continued work on this!

Also @surbhi90, please provide more detail on what you reviewed when post a comment in order to receive issue credit and help the issue progress. Thanks!

This does not appear to be an issue anymore? Without specific steps it was a little bit difficult to get to the bottom of it, especially because the IS has no info other than screenshots. From what I can see it appears the same in LTR and RTL in Seven and Bartik and I think it looks fine:

Bartik LTR:

Bartik RTL:

Seven LTR:

Seven RTL:

So I will close it for now. If someone has additional info and steps to reproduce, feel free to reopen.