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.

Sign up for GitHub

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

Jump to bottom

Ensure asset and page titles are correctly shown in ckeditor #13678

Open
wants to merge 3 commits into
base: 5.x
Choose a base branch
from
Open

Ensure asset and page titles are correctly shown in ckeditor #13678

wants to merge 3 commits into from

Conversation

mollux
Copy link
Contributor

@mollux mollux commented Apr 20, 2024

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #13676

Description:

Fixes #13676

sidenote
This whole logic looks weird for multiple reasons, and should be reworked in M6 to remove a lot of unneeded code and logic:

  • the a: prefix is unused, it's just stripped without taking it into account
  • the links are added in the token dropdown, but IMO there should be a diffet one
  • there is no context besides the title what the reference link is, it can be both a page and an asset

Steps to test this PR:

see #13676

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

@mollux mollux added T1 Low difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs editor Anything related to CKEditor labels Apr 20, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 20, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.30%. Comparing base (f86c50e) to head (9d49b6c).

❗ Current head 9d49b6c differs from pull request most recent head 33fb993. Consider uploading reports for the commit 33fb993 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13678      +/-   ##
============================================
- Coverage     61.50%   61.30%   -0.21%     
+ Complexity    34067    34005      -62     
============================================
  Files          2241     2238       -3     
  Lines        101850   101650     -200     
============================================
- Hits          62647    62315     -332     
- Misses        39203    39335     +132

see 50 files with indirect coverage changes

@RCheesley RCheesley mentioned this pull request Apr 29, 2024
Open
1 task
@RCheesley RCheesley added the ready-to-test PR's that are ready to test label Apr 29, 2024
@RCheesley RCheesley added this to the 5.1.1 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs editor Anything related to CKEditor ready-to-test PR's that are ready to test T1 Low difficulty to fix (issue) or test (PR)
Projects
Open Source Fridays
Status: No status
Milestone
5.1.1
Development

Successfully merging this pull request may close these issues.

Using a colon in an asset title truncates the title in the page builder token listYour bug title goes here!
2 participants
@mollux @RCheesley