Problem/Motivation

Seven's admin styles for dialogs are overriding some of Off Canvas' styles.

Replication steps:

  1. Install the standard profile
  2. Install Content Moderation and commit bf999ed69d76b7cc381cab7f5f03ca19ee2813ea (before a temp fix for this) of Moderation Note
  3. Enable the Editorial Workflow on the Article Content Type
  4. Create an Article then visit the edit for at /node/N/edit
  5. Click the "View notes" local task, note lack of styles

Screenshots:

Before:
Off canvas seven styles before

After:
Off canvas seven styles before

Proposed resolution

Add a few CSS rules to enforce Off Canvas' styles.

Remaining tasks

None.

User interface changes

Off Canvas dialogs will look better in the backend.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#19 off-canvas-2945571-19.patch577 bytesbendeguz.csirmaz
#16 off-canvas-2945571-16.patch2.54 KBbendeguz.csirmaz
#14 off-canvas-admin-testing.patch.txt568 byteslauriii
off-canvas-seven-before.png268.16 KBsamuel.mortenson
off-canvas-seven-after.png265.67 KBsamuel.mortenson
off-canvas-seven-styles.patch2.56 KBsamuel.mortenson
#10 2945571-10.patch2.54 KBjofitz
#11 dialog_white.JPG26.75 KBrp7
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

This looks good to me.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, off-canvas-seven-styles.patch, failed testing. View results

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Needs frontend framework manager review

Adding the framework manager review tag - this patch fixes a serious bug in the off canvas dialog that makes it unusable with the Seven theme. The changes to the Stable theme CSS needs a framework manager's approval, per a conversation I had with @xjm.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, off-canvas-seven-styles.patch, failed testing. View results

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

I hit a similar issue in #2949991: Add workspace UI in top dialog, but fixed it a little differently.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs reroll
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2945571-10.patch2.54 KB

Re-roll.

rp7’s picture

rp7 CreditAttribution: rp7 commented
FileSize
dialog_white.JPG26.75 KB

Patch in #10 does not work for me, still the same issue.

The following CSS rule is what's causing this on my end:

white dialog css rule

If I disable the background: #fff, it looks fine.

rp7’s picture

rp7 CreditAttribution: rp7 commented
Status: Needs review » Needs work
rp7’s picture

rp7 CreditAttribution: rp7 commented

Follow-up: it appears that the patch works when the moderation_note (https://www.drupal.org/project/moderation_note) module is enabled.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
off-canvas-admin-testing.patch.txt568 bytes

It seems like the most recent patch doesn't work with the most recent version of core. It seems like the CSS in Seven shouldn't be applied at all to off-canvas dialogs. Could we make the selectors in Seven more specific so that they wouldn't apply to off-canvas dialogs?

I attached patch that I used for testing the most recent version of the patch. It can be by enabling off_canvas_test module and navigating to /off-canvas-test-links path.

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

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

bendeguz.csirmaz’s picture

bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented
FileSize
off-canvas-2945571-16.patch2.54 KB

White background removed by using a (stronger) id selector.

bendeguz.csirmaz’s picture

bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Component: CSS » Seven theme
Status: Needs review » Needs work

Thanks for a new patch @bendeguz.csirmaz!

This problem is Seven specific bug caused by Seven. Would it be possible to fix this in Seven instead of Stable? According to our backwards compatibility policy, CSS files in Stable are public API and therefore shouldn't be changed without a very good reason.

I'm moving to Seven component since this is a bug in Seven.

bendeguz.csirmaz’s picture

bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented
Status: Needs work » Needs review
FileSize
off-canvas-2945571-19.patch577 bytes

Override from Seven's css.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: -Needs frontend framework manager review

This approach looks better!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks right to me.

  • lauriii committed f0d889e on 8.7.x 
    Issue #2945571 by bendeguz.csirmaz, samuel.mortenson, Jo Fitzgerald,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 8.7.x-dev » 8.6.x-dev

Committed f0d889e and pushed to 8.7.x. Thanks! 🚀 Leaving open against 8.6.x to be considered for backport.

  • lauriii committed 23c105c on 8.6.x 
    Issue #2945571 by bendeguz.csirmaz, samuel.mortenson, Jo Fitzgerald,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Cherry-picked 23c105c and pushed to 8.6.x.

Status: Fixed » Closed (fixed)

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