Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WIP] [3.2] [Form] add 'empty_view' option in FormType#17609
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
[WIP] [3.2] [Form] add 'empty_view' option in FormType#17609
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
looks like the whitespace control changes are unrelated to this feature and should be done in a different PR
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.
@xabbuh I did this to match bootstrap layout. DYTH that it needs a PR ?
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.
Well, I think there's now use in having this different in 2.8/3.0.
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.
@xabbuh I don't understand your 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.
@HeahDude it's not related to this PR so should be done in a separate one. It should also be done on the latest branch this layout is available, to ease future merging.
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.
@jakzal ok reverted.foundation_5_layout was introduced on 2.8 should I make a PR on this branch or on master ? Do you think it's really needed ?
xabbuh commentedFeb 3, 2016
Status: Needs work |
HeahDude commentedFeb 3, 2016
| } | ||
| // Provide a default message when choices var is empty | ||
| if (empty($view->vars['choices']) &&$options['expanded']) { |
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 remove thisif completely. Its up to the template to decided what to display and when.
At least I would remove the check forexpanded since one could decide to display a message insteadof an empty select box.
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.
@backbone87 👍 for removing the expanded condition.
But I don't think it's worth init a view varempty_choices and add it to the context ifchoices are not empty.
d132bbe to18fd85eCompare| <?phpecho$view['form']->block($form,'choice_widget_collapsed')?> | ||
| <?phpendif?> | ||
| <?phpelse:?> | ||
| <?phpif ('' ===$empty_choices) { |
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.
$empty_choices isn't set here (otherwise the block above would have been executed).
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.
right, the condition on the first line is wrong, it should be :+<?php if (!isset($empty_choices)): ?>
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.
@xabbuh thanks, fixed.
HeahDude commentedFeb 18, 2016
Work is still in progress, but I've updated the description and refactored a bit in the last commit. Status: Needs Review |
HeahDude commentedFeb 22, 2016
I've got a suggestion as there is here an addition of a new default translation domain, may be it would be nice to translate the Do you agree with this handling ? |
| <body> | ||
| <trans-unit id="1"> | ||
| <source>No choice available</source> | ||
| <target><em>No choice available.</em></target> |
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.
Either the em-tag should be put in the correct xmlns or the whole content of target should be marked CDATA
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.
@backbone87, thanks for the review, I've just copy-pastedvalidators.en.xliff as I'm not familiarized with this format, could you please precise ?
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.
its nothing specific to xliff. its about how xml namespaces work. when an xml parser parses the given xliff xml file, it interprets the em-element to be part of the xliff namespace (which is declared as the "default" namespace in the given document withxmlns="urn:oasis:names:tc:xliff:document:1.2"). This isnt a problem, unless the xliff consumer doesnt know what todo with an em-element. But xsd validation against the xliff schema will most likely fail (because it probably didnt define an em-element).
One way is to declare the xhtml namespace in the document ´xmlns:xh="http://www.w3.org/1999/xhtml"` and qualify the em-element against it:<xh:em>...</xh:em>.http://www.w3.org/1999/xhtml is used for xhtml 1.x and xml serialisation of html5 (xhtml5).
But since we dont care for the structure of the content in this use case, because the translation consumer expectstext/plain produced by xliff, we can just properly escape the content of the target-element. either with<target><em>No choice available.</em></target> or (more readable)<target><![CDATA[<em>No choice available.</em>]]></target> (cdata = character data)
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.
@backbone87 Thank you very much for this explanation :)
webmozart commentedMar 14, 2016
I think it's a good idea to add this functionality in general. When choices are loaded from a data source and no choice is available, displaying some text instead seems like the proper thing to do. I'm not happy with the naming though. "empty", in the sense of a choice field, refers to the "empty choice", i.e. the choice that is selected ifno choice is selected. Using the same terminology creates ambiguity and potentially confuses our users. Unfortunately I can't think of a better name at the moment. |
HeahDude commentedMar 14, 2016
@webmozart, I proposed We could add a default normalizer at the root definition for more safety to no use it when |
webmozart commentedMar 25, 2016
Even then You are introducing a different concept of "empty", namely that a form is empty if doesn't contain children. Now it could happen that an empty form (your definition: no children) is not empty (existing definition: data is not I think we need to find another name that does not involve the word "empty". |
HeahDude commentedMar 25, 2016
@webmozart, I've kept working in this, and did not use that idea but another one also with "empty"... While implementing it globally, it felt more natural to use What about I just need to fix some tests with table layout and I will push everything. |
HeahDude commentedMar 25, 2016
Updated title and description accordingly. |
| <body> | ||
| <trans-unit id="1"> | ||
| <source>No choice available</source> | ||
| <target>![CDATA[<em>Aucun choix n'est disponible.</em>]]</target> |
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.
the cdata tags have syntax errors. correct is:<![CDATA[ and]]> (not escaped)
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.
@backbone87 thanks, I will update it.
HeahDude commentedMar 30, 2016
I'm actually still stuck on that test: Does anyone know what I need to fix here ? :( |
f68e1e0 to94f2a70CompareHeahDude commentedMar 30, 2016
Rebased. |
HeahDude commentedMar 30, 2016
ping@webmozart :) There is no rush to merge any of my PR, we should polish anything but I can't go further for now without your review I guess. Actually I've got three path tests failing on this one, and I can't make them pass... |
| {%-setattr=attr|merge({class: (attr.class|default('') ~' form-control')|trim}) %} | ||
| <div {{block('widget_container_attributes') }}> | ||
| <p> | ||
| <em>{{-translation_domainissame as(false)?empty_view:empty_view|trans({},translation_domain) -}}</em> |
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.
Why<em>? I think we should refrain from adding styling as much as possible.
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.
OK
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.
em isnt about styling? its about emphasis. and because there is no form, that one might expect, you put the emphasis on the presented message, probably giving reasons as forwhy there are no children.
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 agree, I did not even think about it when I added it naturally.
Because, it may be unexpected to see that string standing for a missing expectation.
Theemphasis clearly mark the informational sense of such string IMHO.
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.
IMO we should leave that decision up to the developer. We never made such assumptions in any other part of form themes.
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.
Fair enough then :)
HeahDude commentedApr 28, 2016
Status: needs work |
nicolas-grekas commentedSep 26, 2017
ping@HeahDude, status of this PR considering feat. freeze? |
fabpot commentedJan 24, 2018
@HeahDude Do you think you can work on finishing this PR or shall we close it? |
fabpot commentedMar 31, 2018
Closing this PR as there is no more activity.@HeahDude Feel free to reopen whenever you have time to finish it. Thanks. |
lukepass commentedJan 9, 2019
Hello, sorry for reopening this one but I had a problem today with an empty CollectionType and I had to manually set setRendered in order to avoid the empty template. Is there another way to fix it? Thanks! |
Uh oh!
There was an error while loading.Please reload this page.
The
empty_viewoption can be astring, or aboolean. By default it is'No field'when compound andfalseotherwise. It uses thetranslation_domainoption for its translation.CollectionTypeinherits it but this can be avoid be settingempty_viewtofalse.The
empty_viewis defined inFormType::finishView()and overriden inChoiceType::finishView()to be defined whenchoicesandpreferred_choicesview vars are empty.In the templates, the old behaviour is kept explicitly set to
false.So 2 ways of using it:
divor an emptyselectinput :empty_rowto use customhtml:What do you think ?