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

Do not add scheme for empty or anchor links #66

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

JoryHogeveen
Copy link
Contributor

@JoryHogeveen JoryHogeveen commented Jan 17, 2022

Questions Answers
Description? Fixes issue with empty links.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#27399
How to test? Create an image slider containing images without URL. Currently the links open a blank page with error. This PR fixes that.

ps_imageslider.php Outdated Show resolved Hide resolved
Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

A little question to be sure :)

@PierreRambaud
Copy link
Contributor

One more approve and I think we can merge :)

@kpodemski
Copy link
Contributor

kpodemski commented Jan 18, 2022

Hi @JoryHogeveen

In this file:

<a href="{$slide.url}">

There's no check if the URL exists or not. I think the point of this PR is to prevent having unused <a> too?

There's no if in the classic too:
https://github.com/PrestaShop/PrestaShop/blob/develop/themes/classic/modules/ps_imageslider/views/templates/hook/slider.tpl#L36

@JoryHogeveen
Copy link
Contributor Author

JoryHogeveen commented Jan 18, 2022

Hi @kpodemski

It definitely is somewhat related and on my client's installation I did remove it if there is only an anchor link.
However, in the current plugin the URL of a slider is set as a required field so I'm not sure if this is intended or not.
This PR merely fixes anchor links and isn't really related to whether it should show a link or not.

@JoryHogeveen JoryHogeveen changed the title Do not add scheme for empty links Do not add scheme for empty or anchor links Jan 18, 2022
@florine2623
Copy link
Contributor

Hello @JoryHogeveen ,

Is there an issue related to this PR ?
If yes, you could link ?
If not, could you create it ?

Thanks!

@JoryHogeveen
Copy link
Contributor Author

@florine2623
Issues are disabled for this repository.

@florine2623
Copy link
Contributor

Hello @JoryHogeveen ,

You can add the issue on the PrestaShop repository : https://github.com/PrestaShop/PrestaShop/issues
The label Imageslider will be added to the issue once reviewed ^^
Thanks!

@JoryHogeveen
Copy link
Contributor Author

JoryHogeveen commented Jan 19, 2022

Done: #27399

@florine2623
Copy link
Contributor

Thanks for the update !

Although I'm having an issue after installing the PR :
Screenshot 2022-01-20 at 15 06 11

Could you check ? ^^
Thanks!

@JoryHogeveen
Copy link
Contributor Author

JoryHogeveen commented Jan 20, 2022

@florine2623

Such installation issues are unrelated to this PR.
I assume it's an environment issue.

In any case, as you can see in the PR, it merely adds 3 lines of code in a method.

Maybe one of the PS core dev's can help you here.

@kpodemski
Copy link
Contributor

@florine2623

This PR is ready to test.

@florine2623
Copy link
Contributor

Hello @JoryHogeveen ,

Thanks for the PR!
Works like a charm ! QA ✅

@Progi1984 Progi1984 merged commit fa1ba71 into PrestaShop:dev Jan 31, 2022
@Progi1984
Copy link
Member

Thanks @JoryHogeveen & @florine2623

@Progi1984 Progi1984 added this to the 3.1.1 milestone Jan 31, 2022
@Progi1984 Progi1984 added the bug label Apr 11, 2022
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.

Anchor links for image slider are incorrectly formatted
5 participants