Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close #3741 Add contextual links to embedded media. #3844

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

trackleft
Copy link
Member

@trackleft trackleft commented Oct 26, 2024

Description

Adding patch from https://www.drupal.org/project/drupal/issues/3174252

We should probably give this patch a good once over and commit back any findings.

Enabling contextual links on embedded media

https://git.drupalcode.org/project/drupal/-/merge_requests/5626/diffs

Release notes

Users with the content editor role can now access contextual links for embedded media to make editing embedded media much easier.

Related issues

Close #3741

How to test

Test on Probo https://66cb76c1-7410-4276-a892-516ff508dfc1.probo.build/news/university-arizona-unveils-pioneering-treatment-type-1-diabetes-game-changer-patients

Screen.Recording.2024-10-25.at.7.20.38.PM.mov

Types of changes

Arizona Quickstart (install profile, custom modules, custom theme)

  • Patch release changes
    • Bug fix
    • Accessibility, performance, or security improvement
    • Critical institutional link or brand change
    • Adding experimental module
    • Update experimental module
  • Minor release changes
    • New feature
    • Breaking or visual change to existing behavior
    • Upgrade experimental module to stable
    • Enable existing module by default or database update
    • Non-critical brand change
    • New internal API or API improvement with backwards compatibility
    • Risky or disruptive cleanup to comply with coding standards
    • High-risk or disruptive change (requires upgrade path, risks regression, etc.)
  • Other or unknown
    • Other or unknown

Drupal core

  • Patch release changes
    • Security update
    • Patch level release (non-security bug-fix release)
    • Patch removal that's no longer necessary
  • Minor release changes
    • Major or minor level update
  • Other or unknown
    • Other or unknown

Drupal contrib projects

  • Patch release changes
    • Security update
    • Patch or minor level update
    • Add new module
    • Patch removal that's no longer necessary
  • Minor release changes
    • Major level update
  • Other or unknown
    • Other or unknown

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires release notes.

@trackleft
Copy link
Member Author

Adding local patch since it seems adding patches to drupal issues is more and more frowned upon these days.

@trackleft trackleft marked this pull request as ready for review October 28, 2024 17:27
@trackleft trackleft requested a review from a team as a code owner October 28, 2024 17:27
@joeparsons
Copy link
Member

Adding local patch since it seems adding patches to drupal issues is more and more frowned upon these days.

Drupal CMS is still doing it:
https://git.drupalcode.org/project/drupal_cms/-/blob/0.x/drupal_cms_events/composer.json

composer.json Outdated
@@ -176,7 +176,8 @@
"filter_autop returns self closing br element with slash, lets alter to br (2350049)": "https://www.drupal.org/files/issues/2024-08-15/2350049-48-reroll-against-11.x.patch",
"[10.2 regression] CKEditor 5 breaks when 'Source'/Source editing button is added and 'Manually editable HTML tags' are specified (3410100)": "https://www.drupal.org/files/issues/2024-01-23/drupal-revert-source-editing-validation-tightening-3410100-38.patch",
"[Apache only] Wrong file header returned, when converting an image for example to webp (3310963)": "https://www.drupal.org/files/issues/2024-05-15/3310963-32.patch",
"Hardcode a higher WebP conversion quality setting (3320689)": "https://gist.githubusercontent.com/joeparsons/d99b6c6eef240e8eaf768ba79e1c9f1b/raw/9b99325bd20907db0506969fc4f5823b46065c6b/3320689-10-3-x-hardcoded.patch"
"Hardcode a higher WebP conversion quality setting (3320689)": "https://gist.githubusercontent.com/joeparsons/d99b6c6eef240e8eaf768ba79e1c9f1b/raw/9b99325bd20907db0506969fc4f5823b46065c6b/3320689-10-3-x-hardcoded.patch",
"Provide option to display contextual links on embedded entities (3174252)": "web/profiles/custom/az_quickstart/patches/composer/drupal/core/3174252-Provide-option-to-display-contextual-links-on-embedded-entities.diff"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we're ready to start using local patches. This path assumes the root level composer is configured a certain way which, although likely, we can't count on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll put a patch on drupal.org

@joshuasosa
Copy link
Contributor

joshuasosa commented Oct 29, 2024

Tested on Probo. Functionally works well in news, events and pages. (That functionality would be nice for gallery paragraphs.)

Here are some issues I encountered:

Contextual filter placement is too far from media embed

When an image is by itself and doesn't fill the width of the containing element, the contextual links placement goes to the far end of the container when I would expect it to be on the image itself.
image

Contextual filter links turn white on dark backgrounds

When an image is on a dark background where text color is changed, the contextual link colors change when they shouldn't.
image

Contextual filters overlap

Contextual filters overlap in certain cases like with news view paragraphs.
image

Contextual filter links are sometimes not usable

There's still an issue with contextual filter links not working properly when they're on top of clickable elements.

When clicking open a contextual filter, the page redirects to the linked element's source instead of letting me use the contextual filter links. Try the News Views Paragraphs demo. Related issue #1519.


Adding local patch since it seems adding patches to drupal issues is more and more frowned upon these days.

Drupal CMS is still doing it: https://git.drupalcode.org/project/drupal_cms/-/blob/0.x/drupal_cms_events/composer.json

It's recommended now to use merge requests instead of patches for Drupal issues (Creating Issue Forks: "the recommended way to contribute source code changes is via merge requests").

Drupal's GitLab contribution docs recommend downloading a patch or diff from the MR since the MR will change as commits are added. They link to a separate page about patches with an example for local patches using a 'patches' directory.

Alternatively, they mention "Consider uploading a patch file to the issue in addition to making a merge request."

In this case, it might be easiest to download the .patch or .diff from the MR and upload it to the issue to use in this PR.

@joeparsons joeparsons added enhancement New feature or request editor experience Improvements to the editor experience for individuals editing Quickstart websites labels Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor experience Improvements to the editor experience for individuals editing Quickstart websites enhancement New feature or request release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add contextual links for embedded media content (i.e. image)
3 participants