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

[WebProfilerBundle] Display the missing translation panel by default #26398

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@javiereguiluz
Member

javiereguiluz commented Mar 4, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#...

Display the "Missing Translations" panel by default ... except if there are no missing translations.

for (var j = 0; j < tabs.length; j++) {
var tabId = 'tab-' + i + '-' + j;
var tabTitle = tabs[j].querySelector('.tab-title').innerHTML;
var tabNavigationItem = document.createElement('li');
tabNavigationItem.setAttribute('data-tab-id', tabId);
if (j == 0) { addClass(tabNavigationItem, 'active'); }
if (hasClass(tabs[j], 'tab-active')) { selectedTabId = tabId; }

This comment has been minimized.

@ro0NL

ro0NL Mar 4, 2018

Contributor

why not check if class 'active' is set already?

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 8, 2018

Member

Your proposal would work, but I think the current one is simpler.

This comment has been minimized.

@ro0NL

ro0NL Mar 8, 2018

Contributor

introducing tab-active + active 🤔 why use 2 "active" classes?

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 8, 2018

Member

Because we use active to apply other styles.

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 9, 2018

Member

I've reviewed this again ... and I was completely wrong. The .active styles are not used how I thought they were, so we can totally reuse that class. @ro0NL thanks for helping me realize this!

@@ -117,7 +117,7 @@
{% endfor %}
<div class="sf-tabs">
<div class="tab">
<div class="tab {{ collector.countMissings == 0 ? 'tab-active' }}">

This comment has been minimized.

@Destroy666x

Destroy666x Mar 8, 2018

Small thing, but wouldn't tab{{ collector.countMissings == 0 ? ' tab-active' }} be more optimal for output? There are redundant spaces in case those fail.

This comment has been minimized.

@stof

stof Mar 8, 2018

Member

We don't care about redundant spaces. They don't hurt at all. And this code is more readable in the template.

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 8, 2018

Member

@Destroy666x you are right, but as @stof said, we always prioritize the readability of Twig templates over the generated HTML code. So let's keep it.

This comment has been minimized.

@Destroy666x

Destroy666x Mar 8, 2018

Ok, I see. IMO both versions are equally readable though

@fabpot

fabpot approved these changes Mar 10, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 10, 2018

Thank you @javiereguiluz.

@fabpot fabpot closed this Mar 10, 2018

fabpot added a commit that referenced this pull request Mar 10, 2018

feature #26398 [WebProfilerBundle] Display the missing translation pa…
…nel by default (javiereguiluz)

This PR was squashed before being merged into the 4.1-dev branch (closes #26398).

Discussion
----------

[WebProfilerBundle] Display the missing translation panel by default

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Display the "Missing Translations" panel by default ... except if there are no missing translations.

Commits
-------

15fe686 [WebProfilerBundle] Display the missing translation panel by default

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

feature symfony#26398 [WebProfilerBundle] Display the missing transla…
…tion panel by default (javiereguiluz)

This PR was squashed before being merged into the 4.1-dev branch (closes symfony#26398).

Discussion
----------

[WebProfilerBundle] Display the missing translation panel by default

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Display the "Missing Translations" panel by default ... except if there are no missing translations.

Commits
-------

15fe686 [WebProfilerBundle] Display the missing translation panel by default

fabpot added a commit that referenced this pull request May 6, 2018

feature #27170 Show the deprecations tab by default in the logger pan…
…el (javiereguiluz)

This PR was merged into the 4.1-dev branch.

Discussion
----------

Show the deprecations tab by default in the logger panel

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

Similar to #26398, I propose to display the deprecation tab by default when there are no error logs but there are some deprecations.

Commits
-------

d27b158 Show the deprecations tab by default in the logger panel

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment