Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Created from https://www.drupal.org/project/drupal/issues/2809635#comment-12433583
The permanent message warning tone is a bit aggressive. Let's calm it down a bit.
Proposed resolution
Some suggestions were change to the appearance and make it visible only on the /admin* pages so if the profile is being used to demo to potential clients, etc it is not visible or obstructing.
Remaining tasks
Define visual changes- DEFERRED, to be addressed by #2938800: Finalize wording for Toolbar warning messageChange the colour slightly- DEFERRED, to be addressed by #2938800: Finalize wording for Toolbar warning messageRemove bold from text- DEFERRED, to be addressed by #2938800: Finalize wording for Toolbar warning message- Make it visible only on admin routes (including node forms)
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff-2938186-34-39.txt | 1.22 KB | smaz |
#39 | 2938186_39_coding_standards.patch | 3.63 KB | smaz |
#37 | Screen Shot 2018-01-29 at 21.30.52.png | 107.85 KB | John Cook |
#37 | Screen Shot 2018-01-29 at 21.30.33.png | 158.72 KB | John Cook |
#35 | interdiff_2938186_33_34.txt | 388 bytes | Eli-T |
Comments
Comment #2
webchickSince this is a beta blocker, escalating to major.
Comment #3
ckrinaI'll work on this. I'll change the colour to #78520D (with #fdf8ed in the background passes AA with 6.5:1).
Comment #4
ckrinaAnd here's the small patch.
Comment #5
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi Ckrina,
Thanks for this patch.
I think we should perhaps make the text not bold/strong. Also, I think there was discussion at the product owners meeting about toning down the colour of the thing, not just the text (background and border as well).
A final consideration was to make this only visible on the admin pages, so people would only see it if they try to change configuration rather than at all times.
Comment #6
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedNot sure if this is any help, but I'll leave it here for discussion.
Comment #7
ckrinaOk Mark! Then this visual changes need to be defined. Updating the issue and unassigning in case anyone wants to work on this before I get time.
Comment #8
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #9
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedReview of patch #8
1. Colour contrast.
The colour of the message is #78520d on #fdf8ed, which has a contrast ratio of 6.6:1, which satisifies WCAG at level AA.
However we aren't using this colour combination anywhere else in Drupal core. The existing style for warning messages in Seven and Classy is slightly stronger: #734c00 on #fdf8ed. This has a contrast ratio of 7.2:1 which satisfies WCAG at level AAA. Can we re-use this combination please?
2. Identifying an admin page
+ if (strpos(\Drupal::service('path.current')->getPath(), 'admin')) {
By testing the path like this, it doesn't match pages like the node edit form, where this message would be very useful.
Here's another way to test for admin pages. It matches the node add/edit pages, and others like
devel/php
too. It's done this way in the patch at #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites.3. Something is up with caching. It might be something to do with the toolbar module itself, or maybe we can set cache metadata in individual toolbar items? The odd behaviour I found was that the message sometimes appeared on the node edit form, sometimes not. Clearing cache using Drush, then reloading the node form resulted in no message being displayed on the node edit page.
Comment #10
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #11
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedTested, the message is showing on admin pages (including node edit forms).
I'm marking this as RTBC and am going to create follow up issues for the actual content and design of the warning message.
Comment #12
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #13
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedFollow up issue created "Finalise design for Toolbar warning message" https://www.drupal.org/project/drupal/issues/2938800
Comment #14
ckrinaSorry, but I'd say to remove the color change from this issue and do it on the Design issue. I'll move Andrew's comment there.
Comment #15
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedIt might be quicker to leave the color change in this issue and then fix it in the follow up one.
If it’s at RTBC at the moment it’ll probably get committed tonight, and then we can sort out the exact colour over the next day or two after the UX meeting tonight.
Or else it means we have to go back to getting the patch together, and then reviewed, and then wait for a commit again. (Just my thoughts)
Comment #16
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe: #14-15
I think @ckrina is right. Better to take the colour change out of scope here, rather than commiting it in haste.
Updating tasks in issue summary.
Comment #17
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedI don't think it's being issued in haste. It's irrelevant to the issue. That could be red, blue, green, it wouldn't matter. The point of this is to set the paths that the message appears. We have another issue to decide the colour(s) to be used.
Comment #18
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAt least, I thought I updated the tasks in the summary.... done now.
Comment #19
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedOops, I failed HTML 101 exam in my last edit.
Comment #20
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHere's a patch to revert the colour back to #000
Comment #21
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedPatch in #20 looks good to me. Manually tested umami-themed content pages, node forms, and generic admin pages. Message also devel/* routes so the is-admin test is correct.
Comment #22
larowlanI don't think we need the inline variables here, and admin context will use route match if no route is passed
so we should only need
Also, we need a test for this.
We already have an existing test that visits a node-edit form, so we can assert the message is present there.
see
\Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testEditNodesByAdmin
Comment #23
webchickSounds like a needs work. :)
Comment #24
larowlanThanks, d.o has been eating my updates all morning
Comment #25
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #26
Eli-TEdited the issue summary to mark *all* visual issues as deferred to #2938800: Finalize wording for Toolbar warning message, to make the remaining element easier to review.
Comment #27
John Cook CreditAttribution: John Cook at Creode commentedI've tested the patch by navneet0693 in #25.
The patch works as designed but we need better tests.
We should move the test for checking the "demonstation" text to a new test and test the following:
/admin/...
page
Comment #28
Eli-TAdded a new patch to implement tests suggested by John Cook in 27, and moved to their own test rather than reusing the edit test.
Also removed static reference to toolbar in DemoUmamiProfileTest::modules static variable; afaik that should be mandated by the umami_demo profile anyway.
Comment #29
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThe list of assertions (pages to visit) look reasonable.
Nitpick for comment style:
/* @var $recipe_node \Drupal\node\Entity\Node */
The preferred form seems to be
/* @var \Drupal\node\Entity\Node $recipe_node */
. It's proposed at #2305593: [policy] Set a standard for @var inline variable type declarations, #2305641: Use the "standard" format for @var inline variable type declarations, and PSR-5, but these are all still at the draft stage. The majority of instances in D8 core already follow the proposal (type first, then name).Comment #31
John Cook CreditAttribution: John Cook at Creode commentedI've got the tests passing - at least locally.
The problem was that the toolbar block was being cached when the tests were running but not when viewing the page. I've put in a cache context for the route.
I've also fixed Andrew's nitpick.
Comment #32
smazI think this needs work:
There is an empty tab at the end of the toolbar items (noticeable due to the bug causing green hover over effects on toolbar items):
I think this is because we always have:
But the tab inside it is conditional:
I think the whole item, not just the tab, needs to be in the check for admin route. Otherwise the toolbar item gets rendered, without the child tab content.
I haven't tested that theory though - I will if I get a chance.
Comment #33
John Cook CreditAttribution: John Cook at Creode commentedI was concentrating on getting the tests to pass.
If the
['#cache']['context']
isn't returned then the toolbar block gets cached and the tests fail.I've moved the type into the conditional as well. This prevents the issue that @smaz found in #32.
Comment #34
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAn empty toolbar item (role) is a no-no for accessibility
Comment #35
Eli-TFound a tiny coding standards issue.
Comment #36
Eli-TInstalling with #35 - have successfully checked the tests pass locally, and that no html element is inserted for the empty tab on pages that do not display the warning.
I did experiment with moving the entire definition of $items['experimental-profile-warning'] but that causes a test to fail, presumably because it breaks the caching.
So this looks good - I'd set it RTBC but I've probably done too much on it myself.
Comment #37
John Cook CreditAttribution: John Cook at Creode commented@andrewmacpherson the patch in #33 removes the empty toolbar item.
The following show the toolbar mark-up with the "admin" item selected.
/admin/content
/
So the (empty) toolbar item isn't rendered using that patch.
Comment #38
Eli-TComment #39
smazAnother nitpick:
We do not need to declare $items in advance - no other core implementations of this hook do so (example from the toolbar module).
I don't think we need to add the 'access toolbar' permission here for just testing editing nodes - I think that should only be in testDemonstrationWarningMessage(). Unless that was needed for some reason @John Cook?
I tested the empty tab issue & can confirm this is now resolved.
Comment #40
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedI think we are in RTBC land.
Comment #41
John Cook CreditAttribution: John Cook at Creode commented@smaz,
@eli-t and I adapted the test from @navneet0693 in #25. We moved the assertion but didn't notice the added permission, so it's not required in that test.
Comment #42
larowlanAdding credits for those who reviewed and impacted the final patch
Comment #43
larowlanCommitted as 41baf34 and pushed to 8.6.x
Comment #47
ahaomar CreditAttribution: ahaomar commentedIs this via module ? i am looking for same functionalities please suggest me Module for that
Comment #49
markconroy CreditAttribution: markconroy at Annertech commentedHi @ahaomar
This is a function inside the umami profile, not a custom module. You can see the commit here: https://git.drupalcode.org/project/drupal/commit/41baf34#206167013fddf92...