@mahmoudmagdy1-1
please move this pr on the new branch 5.x-dev cause is a new feature

thanks in advance

i don't like where the button reset hit it is placed, it should be on the toolbar

i don't like where the button reset hit it is placed, it should be on the toolbar

you mean right here maybe?

yes exaclty, cause i think it's much more "family feeling"

The latest changes I've made changes the button position and it handles the cases that if the weblinks user have set their Count Clicks in the Global Configuation for Weblinks to Off or if the user is making a new weblink, they wouldn't see the button, this is the final result on the front end

The build kept failing so I fixed it and force pushed the commits back to keep the commit history clean, is there an issue with that or that is fine?

you can override the setting of Global Configuration for Weblinks in the weblink item options

so we should not show the reset button if is set to NO in the weblink item options, like is done for the Global Configuration

and in the other way around

so we should not show the reset button if is set to NO in the weblink item options, like is done for the Global Configuration

and in the other way around

It should be able to handle this case now

Thanks for the feedback I really appreciate it, I just addressed the issues you mentioned,
Should I add comments inside those functions? or just stick with the docblock only?

me and @joomdonation have had a discussion about this pull request, and we come to the conclusion that it would better, to put the reset hit button like it has been done for the banners component, sorry for my previuos comment to move the button on the toolbar i've forgot about banners

Also, you made too many unnecessary docblock changes in the PR. Please revert all the changes you made to docblock in this PR. For example, for @param spaces, we use the following rules:

• Between tag and type: 3 spaces
• Between type and name: 2 spaces
• Between name and description: 2 space

me and @joomdonation have had a discussion about this pull request, and we come to the conclusion that it would better, to put the reset hit button like it has been done for the banners component

I didn't know about this, thanks for mentioning it, now after the latest changes I changed the implementation to match the banners component's reset

Also, you made too many unnecessary docblock changes in the PR. Please revert all the changes you made to docblock in this PR. For example, for @param spaces, we use the following rules:

Sorry, I just reverted those changes, thanks for letting me know.

i'm afraid you have removed too much needed files, please check

i'm afraid you have removed too much needed files, please check

It works for me, can you tell me what exactly is not working for you?

Guess it is because you have old modified files left in your installation. Please check the modified files for your PR https://github.com/joomla-extensions/weblinks/pull/590/files , you removed the changes you made in model, controller classes, so the change was lost and won't work anymore.

you removed the changes you made in model, controller classes, so the change was lost and won't work anymore.

The old changes are no longer needed

After debugging now from a fresh install I see why it doesn't work for you, The issue lies within the com_patchtester (it's the same issue i made this pr for joomla-extensions/patchtester#376), The issue is caused because the changes I made contain another "src"

so that's why it doesn't actually apply that change in the joomla-cms, hence why you aren't seeing the changes

i can confirm it works fine
for those want to test the pr with com_patchtester this pr joomla-extensions/patchtester#376 must be applied before

@joomdonation please take a look when you have time

Thanks @mahmoudmagdy1-1 for your work. I expected to have server side code to reset hits of weblink item, not just a litle javascript code to reset value of hits input. By removing unset filter for the field, it opens the door to allow admin to manual adjust the value of Hits (for example, inspect element and change the value manually)

But maybe it is not that important to prevent that behavior, so we will accept this change for now.

Thanks again and sorry for slow getting back to you, I'm quite busy these days. Looking forward to seeing other improvements from you.

Thanks @mahmoudmagdy1-1 for your work. I expected to have server side code to reset hits of weblink item
But maybe it is not that important to prevent that behavior, so we will accept this change for now.

No problem, Yes I've noticed that but that's how is the reset button is implemented in other components, maybe we will fix this behavior in the future,

Thanks again and sorry for slow getting back to you, I'm quite busy these days. Looking forward to seeing other improvements from you.

It's Okay, Thanks for taking the time to review and giving me feedback :)