Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Feature: [Form] Toggle Displaying Percent Type Symbol#28797
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Also update the bootstrap layout unit tests.
Also includes unit test
xabbuh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks quite good so far. I have just left a few comments.
Can we think of use cases where you would want to display another symbol than the percent character (then it could make sense to allow arbitrary strings for this option)?
| * Deprecated the`--no-debug` console option, set the "APP_DEBUG" | ||
| environment variable to "0" instead. | ||
| * Deprecated the`Templating\Helper\TranslatorHelper::transChoice()` method, use the`trans()` one instead with a`%count%` parameter | ||
| * Only display`PercentType` percent symbol if the`symbol` option is`true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we need a changelog entry for this in the FrameworkBundle.
| {%-endblockdateinterval_widget %} | ||
| {%blockpercent_widget -%} | ||
| {%ifsymbol==true %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's shorten this:{% if symbol %} (and everywhere else below too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It also seems like we need to take whitespace control (i.e. the- character) into account here too.
src/Symfony/Bridge/Twig/CHANGELOG.md Outdated
| * add bundle name suggestion on wrongly overridden templates paths | ||
| * added`name` argument in`debug:twig` command and changed`filter` argument as`--filter` option | ||
| * deprecated the`transchoice` tag and filter, use the`trans` ones instead with a`%count%` parameter | ||
| * Update`percent_widget` to consider`symbol` option for`PercentType` when displaying the percent symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we need such an entry in the changelog file of the Twig bridge
| * deprecated calling`FormRenderer::searchAndRenderBlock` for fields which were already rendered | ||
| * added a cause when a CSRF error has occurred | ||
| * deprecated the`scale` option of the`IntegerType` | ||
| * add`symbol` to`PercentType` options and make available as`FormView` vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would reword this a bit:
added a
symboloption to thePercentTypethat allows to disable the output of the percent character
Uh oh!
There was an error while loading.Please reload this page.
| */ | ||
| publicfunctionbuildView(FormView$view,FormInterface$form,array$options) | ||
| { | ||
| $view->vars['symbol'] =isset($options['symbol']) ?$options['symbol'] :true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can be shortened as the option will always be set:
$view->vars['symbol'] =$options['symbol'];
# Conflicts:#src/Symfony/Component/Form/CHANGELOG.md
d42ohpaz commentedOct 10, 2018
I missed finding this yesterday, but apparently there are a fewdifferent versions of a percent symbol:
And there areicons. Unless others object, I'll update the PR later today to allow arbitrary text or boolean false. |
| </div> | ||
| </div> | ||
| {%-else -%} | ||
| {{-block('form_widget_simple') -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could you please add more indention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same for the other twig parts
| {%blockpercent_widget -%} | ||
| {%-ifsymbol==true -%} | ||
| <divclass="input-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could you please add more indention?
d42ohpaz commentedOct 12, 2018
As for the failed unit tests, I could use some guidance on how to set up an environment and/or execute the tests so that I can reproduce their failures. |
xabbuh commentedNov 1, 2018
@dohpaz42 Sorry, I missed your question. I thinkhttps://symfony.com/doc/current/contributing/code/tests.html should provide some useful insights. |
xabbuh commentedFeb 15, 2019
@9ae8sdf76 Do you think you will be able to finish this pull request? |
fabpot commentedMar 4, 2019
@xabbuh Can you take over this one? |
OskarStark commentedMar 4, 2019
I could overtake this too, I just need to know what’s featurewise missing here? |
fabpot commentedMar 4, 2019
@OskarStark Thank you. I think it's ok feature-wise. It's more about rebasing/squashing and fixing tests if needed after that. |
OskarStark commentedMar 4, 2019
Done in#30433 This one can be closed |
PercentType
symboloptionAs of this writing, Symfony will forcibly append a percentage sign (
%) to all input fields that are of thePercentTypeform type. This PR will introduce a boolean flag calledsymbolthat, whenfalse, will not display the percentage sign. Each of the default layouts that definepercent_widgetwill respect this option.By default, this new option will be set to
trueso that it maintains backward compatibility. The unit tests have been updated where appropriate, and a new unit test has been added (as appropriate).