Skip to content

[Form][WebProfiler] Empty form names fix #12267

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Form][WebProfiler] Empty form names fix #12267

wants to merge 1 commit into from

Conversation

kix
Copy link
Contributor

@kix kix commented Oct 20, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR none

When a Form had no name, the markup was broken in the profiler, making the form tree ugly. This pull request changes the output so that (no name) string is displayed when no form name was available.

Before:
screenshot 2014-10-21 00 06 23

After:
screenshot 2014-10-21 00 08 02

@phramz
Copy link
Contributor

phramz commented Oct 20, 2014

won't a   do it as well?

@kix
Copy link
Contributor Author

kix commented Oct 20, 2014

It sure could, but personally I think that it would be less understandable. This fix improves usability a bit, too, IMO.

@javiereguiluz
Copy link
Member

@kix thanks for this PR! I personally love this kind of small fixes that improve the overall quality of the framework. This will lead to less confusion for developers and that's very important.

I also agree that showing some explicit message, such as no name, it's better than displaying a white space.

@xabbuh xabbuh mentioned this pull request Oct 20, 2014
Closed
stof
stof reviewed Oct 21, 2014
View reviewed changes
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/form.html.twig
@@ -420,7 +420,11 @@
{% else %}
<div class="toggle-icon empty"></div>
{% endif %}
{{ name }}
{% if name | length %}
Copy link
Member

Choose a reason for hiding this comment

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

you should remove the spaces around the | according to the Twig coding standards.

And I even suggest using {% if name is not empty %} here. It will make the intent easier to understand

Copy link
Contributor

Choose a reason for hiding this comment

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

@stof 👍

@webmozart webmozart added the Form label Oct 22, 2014
@kix kix force-pushed the profiler_empty_form_names branch 2 times, most recently from 76411e1 to 88c5910 Compare October 27, 2014 10:16
@kix
Copy link
Contributor Author

kix commented Oct 27, 2014

@webmozart Done. Is it better now?

@webmozart
Copy link
Contributor

Yes! :) Could you add the same fix to the details view on the right?

stloyd
stloyd reviewed Nov 3, 2014
View reviewed changes
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/form.html.twig
@@ -420,7 +420,11 @@
{% else %}
<div class="toggle-icon empty"></div>
{% endif %}
{{ name }}
{% if name is not empty %}
{{ name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply:

{{ name|default('(no name)') }}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kix kix force-pushed the profiler_empty_form_names branch from 88c5910 to 3800402 Compare November 4, 2014 12:05
@kix
[Form][WebProfiler] Empty form names fix
c121dea 
When a Form had no name, the markup was broken in the profiler,
making the form tree ugly.
@kix kix force-pushed the profiler_empty_form_names branch from 3800402 to c121dea Compare November 4, 2014 12:15
@kix
Copy link
Contributor Author

kix commented Nov 4, 2014

Got it. Right-side detail view is updated in the same manner, too.

@fabpot
Copy link
Member

fabpot commented Nov 21, 2014

Thank you @kix.

fabpot added a commit that referenced this pull request Nov 21, 2014
@fabpot
bug #12267 [Form][WebProfiler] Empty form names fix (kix)
d88c60d 
This PR was submitted for the master branch but it was merged into the 2.5 branch instead (closes #12267).

Discussion
----------

[Form][WebProfiler] Empty form names fix

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | none

When a Form had no name, the markup was broken in the profiler, making the form tree ugly. This pull request changes the output so that `(no name)` string is displayed when no form name was available.

Before:
![screenshot 2014-10-21 00 06 23](https://cloud.githubusercontent.com/assets/345754/4706329/d596a3ec-5883-11e4-8c67-44a7f357f3e1.png)

After:
![screenshot 2014-10-21 00 08 02](https://cloud.githubusercontent.com/assets/345754/4706359/122674c2-5884-11e4-8237-0177a590f2a0.png)

Commits
-------

6e9642a [Form][WebProfiler] Empty form names fix
@fabpot fabpot closed this Nov 21, 2014
@kix kix deleted the profiler_empty_form_names branch November 26, 2014 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Form
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kix @phramz @javiereguiluz @webmozart @fabpot @stloyd @stof