Skip to content

Add RTL version of sample images #70

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

Closed
wants to merge 7 commits into from
Closed

Add RTL version of sample images #70

wants to merge 7 commits into from

Conversation

mparvazi
Copy link
Member

@mparvazi mparvazi commented Feb 22, 2022

Questions Answers
Description? Add RTL version of sample images and improve image upload.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#27423 & PrestaShop/PrestaShop#27777
How to test? -

@mparvazi
Copy link
Member Author

@PierreRambaud @NeOMakinG

Redo for #67

NeOMakinG
NeOMakinG previously approved these changes Feb 24, 2022
Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

Some feedbacks :)

@mparvazi
Copy link
Member Author

mparvazi commented Feb 28, 2022

@Progi1984

I changed the condition for upload image, so user can upload image just for one language without error.
It's useful when user wants to update an image just for a language.
But when user want to create slide, if user doesn't choose an image for a language (a language other than shop's default language) then it will be empty.

screen-recorder-mon-feb-28-2022-12-48-23.mp4

So what is the best solution?

  1. Let's to keep empty images if user has not chosen an image (there is a warning in form for this situation)
  2. Using shop's default language image for other languages that have not image (For example if user selected image for English, but not choose an image for Persian, then use English image for Persian)

Progi1984
Progi1984 previously approved these changes Mar 1, 2022
@Progi1984 Progi1984 self-requested a review March 1, 2022 08:33
@Progi1984
Copy link
Member

@PrestaShop/product-team Could you check the @mparvazi question and give him an answer ?

@MatShir
Copy link

MatShir commented Mar 24, 2022

@mparvazi I hope you are okay and sorry for the delay :)
Thanks for the suggestion, you make our life easier. I would say option 1, the slider can be used to promote discounts or products depending on the location, and It might be useful for the merchant to be able to manage it by language. If he wants to use the image from the default language, he can still upload it again.
Tell me, what do you think? Hope you are still motivated for the PR.

@mparvazi
Copy link
Member Author

@mparvazi I hope you are okay and sorry for the delay :) Thanks for the suggestion, you make our life easier. If I would say option 1, the slider can be used to promote discounts or products depending on the location, and It might be useful for the merchant to be able to manage it by language. If he wants to use the image from the default language, he can still upload it again. Tell me, what do you think? Hope you are still motivated for the PR.

I'm agree with option one,
AND slide with empty image must be hidden (that means merchant doesn't want to display this slide for a specific language).

I'll work on it to test different situations.

@mparvazi
Copy link
Member Author

mparvazi commented Apr 10, 2022

@MatShir

Changes:
User can upload image for specific languages in slide.
If image is not defined for a language, then doesn't show empty image slide for corresponding language.

Ready for review.

Progi1984
Progi1984 previously approved these changes Apr 12, 2022
@matks
Copy link
Contributor

matks commented May 10, 2022

Dear @mparvazi I'm sorry it seems we forgot to add the "Waiting for QA" label so this PR has stayed here, valided from code point of view but not tested by QA and now it has to be rebased (git conflict) 😢

@matks matks dismissed Progi1984’s stale review May 10, 2022 09:46

Rebase needed

@mparvazi mparvazi mentioned this pull request May 10, 2022
@mparvazi mparvazi closed this May 10, 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.

ps_imageslider - BO Configuration throws an exception
5 participants