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

Last button should be updated like others #33

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

alfonsolr09
Copy link

@alfonsolr09 alfonsolr09 commented Oct 12, 2023

Questions Answers
Description? Modules - Buttons should be updated see PrestaShop/PrestaShop#21361
Type? bug fix / improvement / new feature / refacto / critical
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/Prestashop#{issue number here}.
How to test? Please indicate how to best verify that this PR is correct.

image
LAST BUTTON UPDATED TO YES/ NO

@florine2623
Copy link
Contributor

@alfonsolr09 , Can you specify how to test and add screenshots ?

@alfonsolr09
Copy link
Author

@alfonsolr09 , Can you specify how to test and add screenshots ?

Same PR like #18

@alfonsolr09
Copy link
Author

@florine2623

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

I think "Enabled", "Disabled" works better in this scenario.

cc @Julievrz

@alfonsolr09
Copy link
Author

alfonsolr09 commented Oct 13, 2023

I think "Enabled", "Disabled" works better in this scenario.

cc @Julievrz

I think so too, but since the other two buttons were modified, they should all be the same. It seems unsightly to me that in two of them it says "yes/no" and in another it says "enabled/disabled"

@kpodemski
Copy link
Contributor

I'll let @Julievrz decide what to do there. Maybe best would be to rethink labels for fields from this module.

@alfonsolr09
Copy link
Author

How about "on/off"? Is international and no need translation

@Julievrz
Copy link
Contributor

Hi, what button are we talking about? Can you add a screenshot, please? I don't have enough context to answer. :)

@alfonsolr09
Copy link
Author

image
@Julievrz

@Julievrz
Copy link
Contributor

Julievrz commented Oct 16, 2023

@kpodemski @alfonsolr09 In this case, the easiest thing to do might be to modify the third label to match the construction of the other two (starting with an action verb).

So we would have: "Detect plugins" Yes/No

But I'm wondering why all toggle buttons wording are modified to "Yes/No". Yes/No forces the user to read the label to understand the state of the feature. "Enabled/Disabled", "Activated/Deactivated" or even "On/Off" would be clearer.

@alfonsolr09
Copy link
Author

Throughout the project I think the labels are being modified in that way (yes/no).
I also think it would be clearer by putting "on/off" and it is a universal language and would not need a translation.

@kpodemski
Copy link
Contributor

@MatShir why blocked?

@MatShir
Copy link

MatShir commented Oct 23, 2023

Blocked by wording !

@alfonsolr09
Copy link
Author

Blocked by wording !

What does it mean?

@MatShir
Copy link

MatShir commented Oct 23, 2023

Hi @alfonsolr09,

Blocked by wording !

What does it mean?

Indeed, here we have @Julievrz & @axelvrsc in charge of the overall coherence of the wording. They have their doubts about the choices made by the initiative. The PR is blocked, so they can think about the best content.

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

@alfonsolr09 Please fill out the PR description. :-)

@alfonsolr09
Copy link
Author

@alfonsolr09 Please fill out the PR description. :-)

done!

Hlavtox
Hlavtox previously approved these changes Nov 23, 2023
@Hlavtox Hlavtox dismissed their stale review November 23, 2023 22:58

Dismissing for now, PR technically OK, but maybe original wording is better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

9 participants