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

Article module fix read more title #44136

Merged
merged 8 commits into from
Sep 29, 2024
Merged

Conversation

RickR2H
Copy link
Member

@RickR2H RickR2H commented Sep 25, 2024

Pull Request for Issue #44135

Summary of Changes

Fix the read more title functionality.

Testing Instructions

  • Toggle the function Read More with Title on and off and see if the title is shown or hidden
  • Change the Read More Limit (characters) to 15 to see if the title is shortened,

Actual result BEFORE applying this Pull Request

Toggle Read More with Title not working

Expected result AFTER applying this Pull Request

Toggle Read More with Title is working and truncation of title is working

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on b996b09


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@brianteeman
Copy link
Contributor

chrome_2AW7e91m2w.mp4

@RickR2H
Copy link
Member Author

RickR2H commented Sep 25, 2024

@brianteeman This option only reflects the settings in the module. Article parameters are not respected. This could be a feature we can add in the future. @drmenzelit What do you suggest?

@brianteeman
Copy link
Contributor

brianteeman commented Sep 25, 2024

@brianteeman This option only reflects the settings in the module. Article parameters are not respected.

Yes that is how it should work but it does not.

My point is that the setting in the content component are preventing the setting in the module from working.

check it out yourself.

  1. set the content component options to hide readmore title
  2. set the module to show readmore title

expected behaviour - module shows the readmore title
actual behaviour - module does not show the readmore title

@brianteeman
Copy link
Contributor

<?php echo LayoutHelper::render('joomla.content.readmore', ['item' => $item, 'params' => $item->params, 'link' => $item->link]); ?>

This line is getting the params from the article - it should be getting the params from the module

@RickR2H
Copy link
Member Author

RickR2H commented Sep 25, 2024

@brianteeman please test again.

@brianteeman
Copy link
Contributor

other than comments about the class names this works correctly

@JeroenMoolenschot
Copy link
Member

Thanks Rick!

One small inconsistency

The module now shows > Read more ... Title.
image

While the component shows > Read more: Title
image

Without displaying the title it works both the same

@brianteeman
Copy link
Contributor

Good spot
Without the title it should be using JGLOBAL_READ_MORE
With the title is should be using JGLOBAL_READ_MORE_TITLE

@RickR2H
Copy link
Member Author

RickR2H commented Sep 25, 2024

Thanks for the feedback! Hopefully fixed now.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on e00e280


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

1 similar comment
@JeroenMoolenschot
Copy link
Member

I have tested this item ✅ successfully on e00e280


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@RickR2H
Copy link
Member Author

RickR2H commented Sep 25, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 25, 2024
@Quy Quy added the bug label Sep 26, 2024
@brianteeman
Copy link
Contributor

I think it would be better to do this the way that @rdeutz has done with #43457 - that way you can still use the layout etc

@brianteeman
Copy link
Contributor

brianteeman commented Sep 26, 2024

Removed my positive test as I think this is doing it the wrong way


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@RickR2H
Copy link
Member Author

RickR2H commented Sep 27, 2024

@brianteeman please test the new version.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on a81d79c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@richard67
Copy link
Member

richard67 commented Sep 28, 2024

@RickR2H I've allowed myself to fix the merge conflict which was caused by your other, merged PR #44132 in file "modules/mod_articles/tmpl/default_items.php". What remains is the unresolved conversation above.

@richard67
Copy link
Member

Back to pending as the test by @JeroenMoolenschot has been invalidated by a later change which was not just a branch update. I will restore @brianteeman 's test result as that was just invalidated by a branch update (with a merge conflict resolved by me but that is not relevant for the test result). But the inresolved conversation should be resolved, and it needs a 2nd human test.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 28, 2024
@brianteeman
Copy link
Contributor

I have not tested this item.

Removing my test as the active makes no sense


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44136.

@RickR2H
Copy link
Member Author

RickR2H commented Sep 29, 2024

@Quy could you please test again?

@Hackwar Hackwar merged commit 9f544f9 into joomla:5.2-dev Sep 29, 2024
3 checks passed
@Hackwar Hackwar added this to the Joomla! 5.2.0 milestone Sep 29, 2024
@Hackwar
Copy link
Member

Hackwar commented Sep 29, 2024

I merged this because the latest changes were removing the unrelated active status and the actual code in this PR was successfully tested before. Thank you @RickR2H for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants