When off-canvas dialog is opened on a smaller screen, there's no bottom padding. I thought it should add bottom padding to #drupal-off-canvas to keep it more consistent.

CommentFileSizeAuthor
#23 after_patch_is_applied.png430.33 KBshardulagg
#23 before_patch_is_applied.png160.65 KBshardulagg
#17 after-patch-is-applied.jpg230.93 KBPurencool
#17 before-patch-is-applied.jpg195.19 KBPurencool
#14 add_bottom_padding_to-2902310-14.patch942 byteskiruba karan
#13 alignment issue.png339.93 KBkiruba karan
#12 add_bottom_padding_to-2902310-12.patch954 bytesyogeshmpawar
#8 add_bottom_padding_to-2902310-8.patch497 bytesnyph1337
padding-bottom.png8.69 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

droplet’s picture

droplet CreditAttribution: droplet commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Priority: Normal » Minor

@droplet good catch should fix. Changing to minor though

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Add bottom padding to offcanvas » Add bottom padding to off-canvas dialog
Category: Bug report » Task
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Novice, +CSS novice
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

Daylan’s picture

Daylan CreditAttribution: Daylan commented
Assigned: Unassigned » Daylan
nyph1337’s picture

nyph1337 CreditAttribution: nyph1337 as a volunteer commented
Status: Active » Needs review
FileSize
add_bottom_padding_to-2902310-8.patch497 bytes

I added padding bottom to off-canvas dialog.

Status: Needs review » Needs work

The last submitted patch, 8: add_bottom_padding_to-2902310-8.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: settings_tray.module » CSS

@nyph1337 thanks for the patch. Now that https://www.drupal.org/node/2784443 has been committed the off-canvas.css file has moved.

Actually you need to update 2 files:
core/misc/dialog/off-canvas.css
core/themes/stable/css/core/dialog/off-canvas.css

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Assigned: Daylan » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
add_bottom_padding_to-2902310-12.patch954 bytes

@tedbow - I have added CSS change in both the files core/misc/dialog/off-canvas.css & core/themes/stable/css/core/dialog/off-canvas.css

kiruba karan’s picture

kiruba karan CreditAttribution: kiruba karan at DrupalPartners for Innoppl Technologies Pvt. Ltd commented
Status: Needs review » Needs work
FileSize
alignment issue.png339.93 KB

A patch applied cleanly. But still CSS not reflecting in the interface. Because of CSS order precedence, .ui-dialog-off-canvas .ui-dialog-content styles override by .ui-dialog-off-canvas #drupal-off-canvas. Here I attached the screenshot for the reference.

kiruba karan’s picture

kiruba karan CreditAttribution: kiruba karan at DrupalPartners for Innoppl Technologies Pvt. Ltd commented
Status: Needs work » Needs review
FileSize
add_bottom_padding_to-2902310-14.patch942 bytes

I have added the padding-bottom to .ui-dialog-off-canvas #drupal-off-canvas selector. Please review.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sparrow_1601’s picture

Sparrow_1601 CreditAttribution: Sparrow_1601 commented
Status: Needs review » Reviewed & tested by the community

I have tested the patch and it appears to function properly.

Purencool’s picture

Purencool CreditAttribution: Purencool commented
FileSize
before-patch-is-applied.jpg195.19 KB
after-patch-is-applied.jpg230.93 KB

This patch works fine in chrome. The steps that were taken to review the patch. See attached images above

  1. Installed latest version of drupal 8.6.x-dev Umami Food Magazine profile
  2. Installed Settings Tray
  3. Applied patch
  4. Cleared caches
  5. Inspected element on different screen sizes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs frontend framework manager review

Good find! I've noticed this before. It's much easier to read with this patch.

Adding credit for the testing as well -- thanks @Purencool for the screenshots.

+++ b/core/themes/stable/css/core/dialog/off-canvas.css
@@ -30,7 +30,7 @@
-  padding: 0 20px;
+  padding: 0 20px 20px;

Having the change in Stable is a BC break. In most cases, even for bugfixes, we should only make changes to the module CSS and the internal themes (Bartik, Seven). Tagging for frontend framework manager review for this point.

Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs frontend framework manager review +Needs change record

This could potentially affect someones custom design for the off canvas so we should create a small change record for this. I think it's better to make this change even though there is a small risk involved since this is something that is mainly shown on custom themes, and the bug fix wouldn't be effective if it would be only done for the product themes.

mohit1604’s picture

mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commented
Assigned: Unassigned » mohit1604
mohit1604’s picture

mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commented
Assigned: mohit1604 » Unassigned
Eric115’s picture

Eric115 CreditAttribution: Eric115 at PreviousNext commented
Status: Needs work » Needs review

Created a draft change record here: https://www.drupal.org/node/2945251

shardulagg’s picture

shardulagg CreditAttribution: shardulagg as a volunteer commented
FileSize
before_patch_is_applied.png160.65 KB
after_patch_is_applied.png430.33 KB

Only local images are allowed.
Only local images are allowed.

patch works well.

Given below are the steps i followed to review the patch. Please see the attached images

1. Installed latest version of drupal 8.6.x-dev with Umami Food Magazine profile
2. Installed Settings Tray
3. Applied patch
4. Cleared browser's cache
5. viewed element on different screen sizes

shardulagg’s picture

shardulagg CreditAttribution: shardulagg as a volunteer commented
Eric115’s picture

Eric115 CreditAttribution: Eric115 at PreviousNext commented

@shardulagg, please do not repost other's images and test results and claim it as your own, it is not productive in helping us progress this issue.

jds1’s picture

jds1
he/they
Location Hudson Valley, NY
CreditAttribution: jds1 commented
Status: Needs review » Reviewed & tested by the community

Looks great on 8.6.x! Live from the Nashville 2018 sprint!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Thanks for creating the change record. This still looks good for me. Committed 0da3a9c and pushed to 8.6.x. Thanks everyone!

  • lauriii committed 0da3a9c on 8.6.x 
    Issue #2902310 by kiruba karan, Yogesh Pawar, nyph1337, Purencool,...

Status: Fixed » Closed (fixed)

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