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.
It came up a few times in #2762505: Introduce "outside in" quick edit pattern to core that it's strange for the "Quick edit" link to be at the top of contextual links in the "normal" site view, but in "Edit" mode it moves down below "Configure" (alphabetical order).
This seems like a pretty easy patch for someone to write, so tagging Novice.
Comment | File | Size | Author |
---|---|---|---|
#57 | 2784567-57.patch | 3.76 KB | tedbow |
#57 | interdiff-2784567-55-57.txt | 983 bytes | tedbow |
#55 | 2784567-55.patch | 3.17 KB | tedbow |
#55 | interdiff-53-55.txt | 1.02 KB | tedbow |
#53 | interdiff-48-53.txt | 3 KB | tedbow |
Comments
Comment #2
tedbowComment #3
miteshmapAdded initial patch, Worked on JS to prioritize link on toggle of "EDIT", Works fine when we toggle "Edit" mode on page. But there's some issue when we enable "Edit" Mode and reload the page. At that time the "Quick Edit" link is not being set on top.
Comment #4
tedbow@webchick && @miteshmap I am not seeing the problem. Maybe I don't understand.
Are you seeing the contextual links actually switch order after you click "edit" in the toolbar? For the contextual links for each block?
I always see the contextual links in the same order with "quick edit" after.
Comment #5
webchickSorry, my original bug report was unclear. If you're on your standard D8 site, on the front page, and hover over the contextual links for a node, "Quick edit" will be the first thing in the list. Versus when you're in "Outside-In" mode, the Quick Edit option scoots down and appears below "Configure foo." Instead, it should always be at the top of the contextual links list. Does that make sense?
Comment #6
miteshmap@webchik: For me it's always "configure block" at the top. But yeah your last comment makes it clear that in any case "Quick Edit" should always be on top.
Comment #7
miteshmapComment #8
crasx CreditAttribution: crasx at Bounteous commentedHere is a php attempt. It places all outside-inblock-configure links first by removing it and using array addition. I tried using #weight on the array but it doesn't look like the contextual link render respects it. I didn't dig too deep into that but it appears it could be a bug?
Comment #9
crasx CreditAttribution: crasx at Bounteous commentedComment #10
bendev CreditAttribution: bendev at WebstanZ commentedwill look into this at drupalcondublin2016
Comment #11
scuba_flyComment #12
bendev CreditAttribution: bendev at WebstanZ commentedtested this successfully but needed to slightly reroll patch
Comment #13
jief CreditAttribution: jief commentedpatch #12 works as expected
Comment #14
alexpottWe should add a test for this?
Comment #15
alexpottI meant to assign to needs work for #14
Comment #16
tedbowOk this adds a test that "Quick edit" shows up first.
Also 2 other fixes.
TEST_ONLY patch to prove it fails without change.
Comment #18
tedbowJust TEST_ONLY patched failed which was expected.
Comment #19
20th CreditAttribution: 20th commentedPatch no longer applies because of the c9dbe65 commit. Rerolling updated patch.
Can anyone upload screenshots of before and after for this patch? I do not see any difference in the order of any contextual links when I apply it...
Comment #20
tkoleary CreditAttribution: tkoleary at Acquia commentedLooks correct to me.
Before
After
Comment #21
20th CreditAttribution: 20th commentedRight, thanks @tkoleary!
This patch was already set to RTBC once, it has tests too, all looks good to me.
Comment #22
tstoecklerPlease do not RTBC your own patch.
Comment #23
tstoecklerI guess this has tests, though, now.
Comment #24
tkoleary CreditAttribution: tkoleary at Acquia commentedok, I'll RTBC it. :)
Comment #26
20th CreditAttribution: 20th commentedThat's strange. Rerolling the same patch.
Comment #27
tstoecklerYup, that's the same patch. Thanks!
Comment #28
xjmWhat is the likelihood of reusing this method? Also, it seems like it has some overlap with the code that follws it in
testBlocks()
. Rather than looping twice over the blocks, why not add these assertions in the existing loop?Doesn't the message here contradict what the assertion is actually asserting?
Thanks!
Comment #29
20th CreditAttribution: 20th commentedPostponing this issue until we have fixed #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly which marks
Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks()
as skipped. Until then, this patch cannot be reliably tested.Comment #30
20th CreditAttribution: 20th commentedIn the mean time, here is an updated patch that tries to address @xjm's comments in #28.
Comment #31
tedbowInstead of postponing this issue and all others involving this test why don't we mark the test as incomplete? \PHPUnit_Framework_Assert::markTestIncomplete allows us to do this.
We can move the new asserts to the top of the loop and then skip over all others. Since our new asserts don't involve clicks or press they shouldn't have the random fails.
I added todo's remove these when #2830485 is fixed
Comment #33
tedbowWhoops my bad. fixed patch
Comment #34
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #35
kiwimind CreditAttribution: kiwimind at Investis Digital commentedShould this be commented using the // format? I realise this is ok, but discouraged according to https://www.drupal.org/node/1354#inline
Typo s/contextrual/contextual
Comment #36
tedbow@kiwimind thanks for the review!
re 1.
I think this is standard for comments that are type hinting a var using @var. If you search for "/** @var" in core it is pretty universal.
2. Fixed
Comment #37
tkoleary CreditAttribution: tkoleary at Acquia commentedConfirmed 1
Comment #38
xjmThe module name should either be Settings Tray or outside_in, I think.
These don't follow our coding standards. The comments need to wrap at 80 characters. There also should not be a colon after
@todo
.Isn't the test already skipped earlier, so this test is still not running? We would need to remove the hunk marking it as skipped also, no?
Can we re-upload a test-only patch to prove the coverage?
Also, super-nitpick, but this message contains a comma splice, and I think we can remove the words "in DrupalCI".
Thanks!
Comment #39
tedbow@xjm thanks
1. fixed
2. fixed
3. You are right. My intention in #31 was to remove skip and replace with markTestIncomplete. The idea is we don't need to skip the entire test.
I forgot remove the skip.
Uploaded a TEST_ONLY
"super-nitpick". Super fixed
Comment #40
20th CreditAttribution: 20th commentedAaand.. the blocking issue #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly is in. @tedbow, we can move back to simpler patch now, I guess.
Comment #41
tedbow@20th thanks for reviving this issue!
Yes here is patch that is just re-roll of your patch from #30
It also includes
Also uploading a test only patch.
Since this was RTBC at #37 I think we should be good.
Comment #42
tedbowActually I hadn't committed the machine name fix for #38.1. Fixed
Comment #44
20th CreditAttribution: 20th commented@tebbow
Maybe it would be better to remove the top blank line added between the 'if' and the comment by:
It is not needed there, I think.
And before someone else pointed this, can you reupload both patch and test-only patch in the same comment to make it easier to find the correct one?
Otherwise, the patch looks good, but because I have worked on it as well, I shouldn't be changing it to RTBC.
Comment #45
tedbowOK removed the empty line. also uploaded TEST_ONLY patch
Comment #48
dmsmidtThis came up again at the dev days in Seville while reviewing #2784569-7: Settings Tray Accessibility: Improve tabbing.
I consider the approach in this issue actually as a quick fix, we could make the edit mode even more accessible (see linked comment).
I was only able to test this after a re-roll. Patch attached.
Woop, it works!
To be able to test this you need to clear you session/local storage after applying the patch.
Comment #49
dmsmidtAll previously open issues with this patch are resolved.
Since the current strategy has already been RTBC-ed before and it still works for 8.4.x we can safely RTBC this again.
Comment #50
dbjpanda CreditAttribution: dbjpanda commented@dmsmidt Thanks for the patch. I tested it. And it is working for me. changing status to RTBC.
Comment #51
lauriiiThis isn't really needed and could cause the test to break in future
Comment #52
dbjpanda CreditAttribution: dbjpanda at Google Summer of Code commentedDeleted
if (count($links) > 1) { }
as per #51Comment #53
tedbowThis doesn't actually test anything if the assert calls is removed.
Changing the test check that "Quick Edit" is the first link and it point to the correct href(to not be confused with block_content link in the future)
Had to update providerTestBlocks() to send the real block id so the href is easier to test.
Comment #55
tedbowForgot the href won't be exact match because of DrupalCI sub directory
Comment #57
tedbowTest fix because a switch statement in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks was not updated for the new value of the $block_id parameter.
Comment #58
GrandmaGlassesRopeManComment #60
GrandmaGlassesRopeManComment #62
dmsmidtComment #63
GrandmaGlassesRopeManSo, no more CI errors?
Comment #65
lauriiiLooks good in overall. Thanks for adding the test coverage for this. Committed 0707a70 and pushed to 8.4.x.
Comment #66
tedbow@lauriii thanks for committing this! It seems it is big accessibility improvement re: #48
Also I think this can be backported to 8.3.x because this an experimental module we have been able to keep the module exactly the same for 8.3.x and 8.4.x.
Comment #67
lauriiiGood point @tedbow. Marking for review from another committer.
Comment #68
tedbow@lauriii alright thanks
I started #57 testing against 8.3.x so we can confirm it applies and passes.
Comment #69
star-szrYup, let's commit to 8.3.x as well since it's an experimental module :)
Comment #71
tedbowIt looks to me like DrupalCI marks this as Needs Work because #57 doesn't apply to 8.4.x which it was already committed to.
#57 was also tested against 8.3.x so marking as RTBC again.
Comment #73
lauriiiCherry picked 6d1b62d and pushed to 8.3.x.
Comment #74
tedbow@lauriii Thanks!
Comment #76
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)