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

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

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
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.30%. Comparing base (f86c50e) to head (9d49b6c).
Report is 21 commits behind head on 5.x.

Current head 9d49b6c differs from pull request most recent head 33fb993

Please upload 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 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
Copy link

@Esthertests Esthertests left a comment

Choose a reason for hiding this comment

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

nice work

titles are no longer truncated with the column
image

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels May 17, 2024
Copy link
Contributor

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer left a comment

Choose a reason for hiding this comment

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

Works as described, fixes the bug. Thanks :)

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer added user-testing-passed PRs which have been successfully tested by the required number of people. and removed pending-test-confirmation PR's that require one test before they can be merged labels May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs editor Anything related to CKEditor T1 Low difficulty to fix (issue) or test (PR) user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Status: 🧑🏻‍💻 Needs a code review
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!
4 participants