#2892942: Contextual links support options but not use them to generate links add support for "options" in *.links.contextual.yml

These options are currently being added outside_in_contextual_links_view_alter() but this can be moved to outside_in.links.contextual.yml

CommentFileSizeAuthor
#2 2893978-2.patch1.5 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Title: Use newly support 'options' in contextual links in Settings Tray contextual links » Use newly supported 'options' in contextual links in Settings Tray contextual links
Status: Active » Needs review
FileSize
2893978-2.patch1.5 KB

Here is patch.

but #2892942: Contextual links support options but not use them to generate links might be reverted because it has not test coverage

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Postponed

#2892942: Contextual links support options but not use them to generate links was reverted

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Use newly supported 'options' in contextual links in Settings Tray contextual links » [PP-1] Use newly supported 'options' in contextual links in Settings Tray contextual links
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Title: [PP-1] Use newly supported 'options' in contextual links in Settings Tray contextual links » Use newly supported 'options' in contextual links in Settings Tray contextual links
Status: Postponed » Needs review
tedbow’s picture

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

@lauriii thanks for updating this issue.

Retesting. Hopefully will pass.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Awesome!

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

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

RTBC++

  • Cottser committed 0a42c06 on 8.5.x 
    Issue #2893978 by tedbow: Use newly supported 'options' in contextual...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed 0a42c06 and pushed to 8.5.x. Thanks!

+++ b/core/modules/outside_in/outside_in.links.contextual.yml
@@ -2,3 +2,9 @@ outside_in.block_configure:
+      data-dialog-type: 'dialog'
+      data-dialog-renderer: 'off_canvas'
+      data-outside-in-edit: 'TRUE'

Minor: I don't think any of these needed quotes. This isn't consistent in HEAD at all and I can't find a documented standard so more of a curiosity and not anything worth making a fuss over :) A quick search turned up #2124559: Figure out a standard for booleans in yaml files.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » tedbow

#11 great remark! Actually… I'm pretty sure that 

+++ b/core/modules/outside_in/outside_in.links.contextual.yml
@@ -2,3 +2,9 @@ outside_in.block_configure:
+      data-outside-in-edit: 'TRUE'

This is wrong. This will result in <blah data-outside-in-edit="TRUE">, rather than <blah data-outside-in>. I think this may require a revert plus test coverage. Assigning to @tedbow for decision on that.

I should've noticed this, especially because I worked on the test coverage at #2892942: Contextual links support options but not use them to generate links that explicitly tests this! :( Sorry!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

@Wim Leers ah yes you're right:

<a href="/admin/structure/block/manage/bartik_tools/off-canvas?destination=node" class="use-ajax" data-dialog-type="dialog" data-dialog-renderer="off_canvas" data-outside-in-edit="TRUE">Quick edit</a>

I think a follow-up issue would be fine, assuming that everything still works with the unneeded TRUE value.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Fixed » Reviewed & tested by the community

This should also be committed to 8.4.x, actually.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

@Wim Leers thank you :) 8.4.x was and is frozen for the first alpha but agreed this should go into 8.4.x when possible.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Yep! Hope this is the right status. Perhaps Patch (to be ported) is more accurate?

I don't have a preference.

  • Cottser committed cccc2d4 on 8.4.x 
    Issue #2893978 by tedbow: Use newly supported 'options' in contextual...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Reviewed & tested by the community » Fixed

Cherry-picked as cccc2d4 and pushed to 8.4.x. Thanks!

I created a follow-up to clean up the data types and discuss further: #2899942: Correct data types in outside_in.links.contextual.yml

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2899942: Correct data types in outside_in.links.contextual.yml

Yay, thanks @Cottser!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

Totally meant to add the relation, thank you @Wim Leers :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Hah, np :)

Status: Fixed » Closed (fixed)

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

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! :)