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

Adding CSS classes to pagebreak.php and other improvements #19303

Closed
wants to merge 4 commits into from

Conversation

AndySDH
Copy link
Contributor

@AndySDH AndySDH commented Jan 5, 2018

Updated Pull Request for Issue #14670, now that I got more knowledge on how a PR is made correctly.

Since the navigation for multi-page articles (plugins/content/pagebreak.pagebreak.php) can't be template-overridden (unlike the article navigation in pagenavigation.php, which can), we should make some basic improvements to it in the core. This is what I did for my site and would be beneficial in general:

Summary of Changes

This PR:

  • Adds CSS classes to the prev/next links, so they can be stylized via CSS (currently they have no class)

  • Adds a pagenavcounter also at the bottom of the page.

  • Changes behavior so that the "Previous" Link is not shown when we are on the first page, and the "Next" Link is not shown when we are on the last page. When we are on the first page, there is little point in showing a blank unclickable "prev" link. Sites don't show that. Same thing for when we are on the last page, the "next link" should not be shown.

  • Adds a different language string for these next page/prev page links, as they are not the generic next/prev article links, but they represent next page/previous page. By having a different language string we can individually value these as "PREV PAGE" and "NEXT PAGE".

Documentation Changes Required

None.

@C-Lodder
Copy link
Member

C-Lodder commented Jan 5, 2018

Not sure about others but I dont agree with this. I don't see the point in adding classes if they're not actually going to be used in core.

Instead I think the HTML markup should be moved to a layout so it can be overridden and changed by a user, if need be.

@mbabker
Copy link
Contributor

mbabker commented Jan 5, 2018

Instead I think the HTML markup should be moved to a layout so it can be overridden and changed by a users if need be.

^^ That

@AndySDH
Copy link
Contributor Author

AndySDH commented Jan 5, 2018

Not sure about others but I dont agree with this. I don't see the point in adding classes if they're not actually going to be used in core.

Note that this doesn't just add classes but also makes a lot of other improvements, including change to if conditionals when on first/last page, and new language strings specific for the occurrence.

But feel free to turn it into a layout or to do something different if you wish!

@mbabker
Copy link
Contributor

mbabker commented Jan 5, 2018

It's not necessarily that the changes here are bad, but if you've got a situation where you need to adjust the markup the only option is to duplicate the plugin and maintain it all because the markup is hardcoded into it.

Although to be honest this is one of the plugins I just want to get rid of in 4.0 and just merge all the logic into com_content. Because this plugin literally only supports the article view of that component.

@@ -80,7 +80,7 @@ public function onContentPrepare($context, &$row, &$params, $page = 0)
{
throw new Exception(JText::_('JERROR_PAGE_NOT_FOUND'), 404);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove tabs

}

if ($page > 0)
{
$page_prev = $page - 1 === 0 ? '' : $page - 1;

$link_prev = JRoute::_(ContentHelperRoute::getArticleRoute($row->slug, $row->catid, $row->language) . '&showall=&limitstart=' . $page_prev);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove tab

}

$row->text .= '<ul><li>' . $prev . ' </li><li>' . $next . '</li></ul>';
$row->text .= $prev . $next;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use tab

@@ -87,6 +88,7 @@ JPAGETITLE="%1$s - %2$s"
JPREV="Prev"
JPREVIOUS="Previous"
JPREVIOUS_TITLE="Previous article: %s"
JPREV_PAGE="Prev Page"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort key in alpha order

@@ -75,6 +75,7 @@ JMONTH="Month"
JNEW="New"
JNEXT="Next"
JNEXT_TITLE="Next article: %s"
JNEXT_PAGE="Next Page"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort key in alpha order

@@ -102,6 +103,7 @@ JONLY="Only"
JOPTIONS="Options"
JPREV="Prev"
JPREVIOUS="Previous"
JPREV_PAGE="Prev Page"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort key in alpha order

@rdeutz
Copy link
Contributor

rdeutz commented Jan 6, 2018

Besides the question if the plugin should be removed, the markup has to be in a layout and not hardcoded within the plugin

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/19303

@Quy
Copy link
Contributor

Quy commented Apr 16, 2018

Closing for reasons as stated above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants