Support from Acquia helps fund testing for Drupal Acquia logo

Comments

munzirtaha’s picture

munzirtaha’s picture

Status: Active » Needs review
munzirtaha’s picture

Manjit.Singh’s picture

Assigned: munzirtaha » Manjit.Singh
Manjit.Singh’s picture

Thanks @munzirtaha for working on this.

I think this issue is in seven theme also. Please check before-item-list-class.png

So to correct the margin of .item-list globally, We have to override system.theme.css. I have done the changes in system.theme.css Please review at your end.

Before

alt

alt

After

alt

alt

Manjit.Singh’s picture

forget to attach patch file.

munzirtaha’s picture

Thanks Manjit,
I checked your patch but there are many bugs regarding rtl bullets in drupal 8, mine is different than yours. The seven theme bullets in the LTR version and RTL version are already the same for the login block. Your patch made the RTL version different.

munzirtaha’s picture

Assigned: Unassigned » munzirtaha

I just created another issue at https://www.drupal.org/node/2501957 to discuss the solution

munzirtaha’s picture

Assigned: munzirtaha » Unassigned
Manjit.Singh’s picture

uploading the same patch there.. Please check that.

LewisNyman’s picture

Component: Bartik theme » CSS

This patch is for system CSS, so I'm moving this issue to the CSS component, can we get screenshots in Classy as well?

munzirtaha’s picture

@LewisNyman: Thanks for pointing to test Classy as well. Now, I am pretty sure it's a bartik theme issue and not a system css bug. Attached is another patch tested with Bartik and Classy.

munzirtaha’s picture

Status: Needs review » Needs work

The last submitted patch, 12: core-bartik-Fixing_RTL_item-list_margin-2501843-12.patch, failed testing.

munzirtaha’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
41.67 KB

@LewisNyman: Updated the patch to specify margin-right specifically and here is how it looks in Classy after the patch
Classy

munzirtaha’s picture

Sorry, something has gone wrong with the previous patch, here it's again

munzirtaha’s picture

seems I also need a margin-bottom: 0 so I reverted back to my old patch. Hopefully, this is the last mind change.

Manjit.Singh’s picture

Issue tags: +Needs screenshots

@munzirtaha Thanks for the patch. Now we just need a screenshots of Seven, Bartik and classy.

munzirtaha’s picture

@Manjit.Singh: Thanks for your review but before taking this any further, I need your opinion on issue #2509902: Remove block.css list styles and add the code to other components. which if got accepted or a modified version of it, could render this patch obsolete, what do you think?

emma.maria’s picture

@munzirtaha It's OK we can carry on with this issue. This is an easy win and I can see #2509902 requiring more thinking and patch iterations.

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Closed (cannot reproduce)
Issue tags: -Needs screenshots
FileSize
14.59 KB
12.64 KB

This issue #2548805: Add sensible base layout styles for lists in Bartik. tidied up list styles and fixed a lot of problems in Bartik. Therefore the problem in this issue is no longer occurring.