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

[4.0] Pagination set to bottom after voting #32914

Closed
wants to merge 5 commits into from
Closed

[4.0] Pagination set to bottom after voting #32914

wants to merge 5 commits into from

Conversation

rjharishabh
Copy link
Contributor

@rjharishabh rjharishabh commented Mar 28, 2021

Pull Request for Issue #32909.

Testing Instructions

Enable Content - Vote plugin with Bottom option selected
Enable Voting in Article options
Visit an article

Actual result BEFORE applying this Pull Request

content then pagination then voting

Expected result AFTER applying this Pull Request

content then voting then pagination

ppp

Documentation Changes Required

No

@rjharishabh rjharishabh changed the title Pagination set to bottom after voting [4.0] Pagination set to bottom after voting Mar 28, 2021
@rjharishabh
Copy link
Contributor Author

@PhilETaylor please test this PR

@PhilETaylor

This comment was marked as abuse.

@rjharishabh
Copy link
Contributor Author

@PhilETaylor Thnx for testing

@ghost
Copy link

ghost commented Mar 29, 2021

@SharkyKZ can i ask you please why you sometimes give a "thumps-down" without a comment? i'm interested about reasons for thumps-down but without a word its for me a strange behaviour.

@rjharishabh
Copy link
Contributor Author

@SharkyKZ can i ask you please why you sometimes give a "thumps-down" without a comment? i'm interested about reasons for thumps-down but without a word its for me a strange behaviour.

same question @SharkyKZ

@PhilETaylor

This comment was marked as abuse.

@rjharishabh
Copy link
Contributor Author

Please test this PR @ceford

@ceford
Copy link
Contributor

ceford commented Mar 29, 2021

I have tested this item ✅ successfully on 0e1f307

I had to enable the Vote plugin and position it below - then the PR works as described and it is clearly more logical for Vote to appear above Next/Previous than below.


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

@rjharishabh
Copy link
Contributor Author

rjharishabh commented Mar 29, 2021

Thnx for testing

@Quy
Copy link
Contributor

Quy commented Mar 29, 2021

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 29, 2021
@Quy Quy added this to the Joomla 4.0 milestone Mar 29, 2021
@Quy Quy removed the RTC This Pull Request is Ready To Commit label Mar 29, 2021
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Mar 29, 2021
@Quy
Copy link
Contributor

Quy commented Mar 29, 2021

I assume by moving it that it is no longer afterDisplayContent.

@rjharishabh
Copy link
Contributor Author

I assume by moving it that it is no longer afterDisplayContent.

I think it's related to voting

@rjharishabh
Copy link
Contributor Author

@Quy
is there anything that I have to change

@infograf768
Copy link
Member

There is a reason @SharkyKZ thumbed down... Just add an image in the article and look at the stars placement.

Before patch

Screen Shot 2021-04-01 at 12 16 49

After patch

Screen Shot 2021-04-01 at 12 10 25

@PhilETaylor

This comment was marked as abuse.

@joomdonation
Copy link
Contributor

Trust me there was not... he just went to all my PR's and did it. its obviously personal. He did it to a load of other PRs within seconds too.

I don't think so. I don't know why he is not happy with Joomla! now but from what I see, most of the time, @SharkyKZ is usually right with his thumb down.

@PhilETaylor

This comment was marked as abuse.

@joomdonation
Copy link
Contributor

If I get his thumb down for my PR, I would look at my code again to see if I could make any mistake :D.

@rjharishabh
Copy link
Contributor Author

He should be given some feedback on how to improve

@infograf768
Copy link
Member

All the PR did was move the stars from after the pagination (which is completely the wrong place for it) to above the pagination. If then the css needs tweaking at a template level it should be.

So be it, but in the same PR. It may also need to insert the code into a div.

@PhilETaylor

This comment was marked as abuse.

@rjharishabh
Copy link
Contributor Author

rjharishabh commented Apr 14, 2021

@infograf768 @Quy
please test

It's looking better now.

@ghost
Copy link

ghost commented Apr 16, 2021

I have tested this item ✅ successfully on 9c699ae

image


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

@infograf768
Copy link
Member

This needs some margin/padding when no images, displayed in blog and placed at bottom.

Screen Shot 2021-04-16 at 10 15 15

@ghost
Copy link

ghost commented Apr 16, 2021

@infograf768 i had made an issue #33164

@rjharishabh
Copy link
Contributor Author

when I am testing the output is this

page

@ghost
Copy link

ghost commented Apr 16, 2021

@rjharishabh you have to look at the blog-menu, not the article-view.

@sandewt
Copy link
Contributor

sandewt commented Apr 16, 2021

when I am testing the output is this

Are you using the latest version of J4? In the last few days several changes have been merged to the vote plugin !

@rjharishabh
Copy link
Contributor Author

when I am testing the output is this

Are you using the latest version of J4? In the last few days, several changes have been merged to the vote plugin!

This branch is not updated.
How can I update this branch with the latest changes? Please help @sandewt

@sandewt
Copy link
Contributor

sandewt commented Apr 16, 2021

How can I update this branch with the latest changes? Please help @sandewt

See: https://docs.joomla.org/J4.x:Setting_Up_Your_Local_Environment

  • Install: composer, node.js and git

In Windows, I use the cmd prompt to clone the latest Joomla 4 version.

git clone https://github.com/joomla/joomla-cms.git -b 4.0-dev /xampp/htdocs/bugtesting/joomla

I would like to hear if it could be simpler !?

@rjharishabh
Copy link
Contributor Author

the 4.0-dev branch is updated with new changes
but the PR branch pagination is not updated
So how can I pagination branch with the new changes?

@drmenzelit
Copy link
Contributor

I don't think this PR is correct: changing the position of afterDisplayContent has effects on all plugins using this event. For me the "pagination" = navigation between articles is part of the content, so afterDisplayContent should be below the pagination.

@brianteeman
Copy link
Contributor

I don't think this PR is correct: changing the position of afterDisplayContent has effects on all plugins using this event. For me the "pagination" = navigation between articles is part of the content, so afterDisplayContent should be below the pagination.

Correct. Changing it here will change every plugin using afterDisplayContent

@PhilETaylor

This comment was marked as abuse.

@ghost
Copy link

ghost commented Apr 16, 2021

the voting should be before the pagination

agree

@drmenzelit
Copy link
Contributor

The implementation in this PR of the fix for #32909 may be incorrect, but Im sure we can all agree that the voting should be before the pagination and not after it like it is currently as per the screenshots here #32909

I agree with that, but we have to find another way than moving the afterDisplayContent

@rjharishabh
Copy link
Contributor Author

The implementation in this PR of the fix for #32909 may be incorrect, but Im sure we can all agree that the voting should be before the pagination and not after it like it is currently as per the screenshots here #32909

I agree with that, but we have to find another way than moving the afterDisplayContent

Yes, I agree

@PhilETaylor

This comment was marked as abuse.

@sandewt
Copy link
Contributor

sandewt commented Apr 17, 2021

Has anyone looked at joomla 3 and worked out how it's done there?

The same issue, see image.

issue_vote

@sandewt
Copy link
Contributor

sandewt commented Apr 17, 2021

I don't think this PR is correct: changing the position of afterDisplayContent has effects on all plugins using this event. For me the "pagination" = navigation between articles is part of the content, so afterDisplayContent should be below the pagination.

What do you think of this minor adjustment? afterDisplayContent direct before pagination

Now

if (!empty($this->item->pagination) && $this->item->paginationposition && $this->item->paginationrelative) :
	echo $this->item->pagination;
?>
<?php endif; ?>
<?php // Content is generated by content plugin event "onContentAfterDisplay" ?>
<?php echo $this->item->event->afterDisplayContent; ?>

Proposal

<?php // Content is generated by content plugin event "onContentAfterDisplay" ?>
<?php echo $this->item->event->afterDisplayContent; ?>
<?php
if (!empty($this->item->pagination) && $this->item->paginationposition && $this->item->paginationrelative) :
	echo $this->item->pagination;
?>

@rjharishabh
Copy link
Contributor Author

rjharishabh commented Apr 18, 2021

@sandewt This PR is the same as your proposal.

@sandewt
Copy link
Contributor

sandewt commented Apr 18, 2021

@sandewt This PR is the same as your proposal.

Yes, but the voting (content plugin) is loaded immediately before the pagination. So, no other actions are happening in the meantime. Or am I wrong?

[EDIT] Oops, I cannot reproduce my suggestion 😅

@sandewt
Copy link
Contributor

sandewt commented Apr 23, 2021

This needs some margin/padding when no images, displayed in blog and placed at bottom.

#33164 (comment)

@rjharishabh rjharishabh deleted the pagination branch April 27, 2021 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants