I often experience the problem with the Save button on the end of a admin page to be cut, so it's hard to see and hard to hit (especially since in some browsers the URL of a link you hover is shown in that area as well).

See screen shot: http://screencast.com/t/u9qXwKmfGfpl
The screen show is taken on a site where I have scrolled down to the bottom of the page. It's from Firefox, but the problem is in Chrome, Safari and probably other browsers as well.

It can be solved by changing the padding in modules/overlay/overlay-child.css from

#overlay {
  display: table;
  margin: 0 auto;
  min-height: 100px;
  min-width: 700px;
  position: relative;
  padding: .2em;
  padding-right: 26px;
  width: 88%;
}

to:

#overlay {
  display: table;
  margin: 0 auto;
  min-height: 100px;
  min-width: 700px;
  position: relative;
  padding: .2em 26px 2em .2em;
  width: 88%;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yoroy’s picture

Version: 7.0 » 8.x-dev

Thanks for the report, looks like an issue with overlay module.

Mac_Weber’s picture

Assigned: Unassigned » Mac_Weber

preparing the patch

Mac_Weber’s picture

Mac_Weber’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, added-padding-to-overlay-1153122-2.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, added-padding-to-overlay-1153122-2.patch, failed testing.

Mac_Weber’s picture

Mac_Weber’s picture

Mac_Weber’s picture

Dave Reid’s picture

Status: Needs work » Needs review
Mac_Weber’s picture

Status: Needs review » Active
Mac_Weber’s picture

Status: Active » Needs review
cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Your efforts in battling the Testbot were valiant. I think this patch is ready to go.

Mac_Weber’s picture

Thanks everybody at #drupal-contribute for helping! This is my first patch.
I think now I learned the tricks.

tim.plunkett’s picture

FileSize
744 bytes

Added leading 0 to decimal values. s/ (\.\d)/ 0\1/g

No patch credit, Mac_Weber did all the work. Just trying to keep the CSS clean.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1153122-16.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review

#16: drupal-1153122-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1153122-16.patch, failed testing.

Bojhan’s picture

Issue tags: +Usability, +Needs screenshots

Can we have some compareable screenshot?

cweagans’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs screenshots

Looks like we're all green, and the patch in #16 looks good. I don't think a screenshot is needed for this one. It's just adding a bit of padding to the bottom of the overlay.

Mac_Weber’s picture

why #16 reports success, but #17 reports 'failed testing' for the same file?

cweagans’s picture

I didn't see the error before it was retested - it was likely that it was a "failed to checkout drupal" or "failed to apply patch" error. It's all green now though, so nothing to worry about - all the tests pass and this change is not going to break anything.

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

It needs a comparable screenshot, please don't deprive me from reviewing this issue - it did not get a visual review, it is my job to give one.

Mac_Weber’s picture

Issue tags: -Needs screenshots
FileSize
27.74 KB

Screenshot:

aspilicious’s picture

Please wait until overlay rtl is comitted. This needs rtl styling. Thnx! :)

Mac_Weber’s picture

How it could affect rtl?

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

This is not blocked on rtl. If this goes in first, the rtl patch needs to account for this patch. If rtl goes in first, this patch should account for rtl.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

This *was* blocked on that patch. It got commited yesterday. Now this patch doesn't apply anymore. And yes it needs to be tested in rtl too.

Mac_Weber’s picture

Status: Needs work » Reviewed & tested by the community

@aspilicious the only patch related to rtl I see commited is http://drupal.org/node/766458 (Seven theme lacks rtl styling), which clearly states: 2) This patch doesn't fix the overlay, thats a seperate issue

This patch looks good to go.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

Webchick forgot to commit a file in http://drupal.org/node/766170
But either way this pathc doesn't apply anymore because that one is fixed.

Srry this is needs work...

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
417 bytes

tested with RTL.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/modules/overlay/overlay-child.cssundefined
@@ -16,8 +16,7 @@ html.js body {
+  padding: 0.2em 26px 2em 0.2em;

Is 26px the same as 0.2em?
Probably not so this needs RTL styling.

Why mixing pixels and em? I would choose em. So 26px needs to be converted.

23 days to next Drupal core point release.

Mac_Weber’s picture

That 26px were added for RTL in http://drupal.org/node/766170
That's why I did not change it.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
681 bytes

added bottom padding separated, as other commits to D8.x had done to other paddings

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

That looks good. I think this should apply correctly to both D7 and D8.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

cweagans’s picture

Congrats on your first core patch, Mac_Weber! Patch got applied to two branches, too! :)

Automatically closed -- issue fixed for 2 weeks with no activity.