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

Remove useless checkbox in product options tab for attached files #16271

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@marionf
Copy link
Contributor

marionf commented Nov 5, 2019

Questions Answers
Branch? develop
Description? Remove useless checkbox in product options tab for attached files
Type? bug fix
Category? BO
BC breaks? Does it break backward compatibility? no
Deprecations? Does it deprecate an existing feature? no
Fixed ticket? Fixes #9790
How to test? Edit a product, go to options tab, add 2 attached files, verify there is no checkbox before "Title"

This change is Reviewable

@marionf marionf requested a review from PrestaShop/prestashop-core-developers as a code owner Nov 5, 2019
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 5, 2019

Hi, Thank you for your contribution. Please can you remove the JS relative to the ID #product-attachment-files-check in admin-dev/themes/default/js/bundle/product/form.js ?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 5, 2019

Hi, Thank you for your contribution. Please can you remove the JS relative to the ID #product-attachment-files-check in admin-dev/themes/default/js/bundle/product/form.js ?

Euuuuh 😅 are we sure this JS is useless ?

@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 5, 2019

Hi, Thank you for your contribution. Please can you remove the JS relative to the ID #product-attachment-files-check in admin-dev/themes/default/js/bundle/product/form.js ?

Euuuuh sweat_smile are we sure this JS is useless ?

It seems there is only one element with ID product-attachment-files-check.

@matthieu-rolland

This comment has been minimized.

Copy link
Contributor

matthieu-rolland commented Nov 5, 2019

Hi, Thank you for your contribution. Please can you remove the JS relative to the ID #product-attachment-files-check in admin-dev/themes/default/js/bundle/product/form.js ?

Euuuuh 😅 are we sure this JS is useless ?

Yes, it's only related to this checkbox and will never be executed when the checkbox is gone.
The only thing I'm not sure of, is if this template is used only once or in other places...

If the checkbox has never a reason to exist then this PR is fine (there's just the JS related to the checkbox to be removed as well)

@marionf

This comment has been minimized.

Copy link
Contributor Author

marionf commented Nov 5, 2019

Done, I deleted the related JS

@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 5, 2019

Done, I deleted the related JS

Thanks @marionf. Don't forget to build assets.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 5, 2019

@Progi1984 No need to build asset for this file

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 5, 2019

@matthieu-rolland @Progi1984 I know the JS is useless if we remove the ID. What I mean is: are we sure they are both useless ? 😛 I find it hard to believe there is a JS + an HTML item that were added and are completely useless. I'm quite sure there is an expected behavior that rely on them.

So removing them is valid only if:

  • the behavior is now obsolete
    or
  • the behavior is broken (then maybe we should fix it instead ?)
    or
  • this is dead code
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 5, 2019

I think it's a dead code, we are talking about our backoffice theme, if something is not needed anymore, we must remove it.
It's not a BC since it's not a part of the Public API, it's a UI / UX problem :)

@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Nov 6, 2019
@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Nov 6, 2019
@PierreRambaud PierreRambaud merged commit dd5d1a8 into PrestaShop:develop Nov 6, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 6, 2019

Thanks @marionf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.