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

[RFC] Deprecate the web_profiler.position option #23528

Closed
javiereguiluz opened this issue Jul 16, 2017 · 5 comments
Closed

[RFC] Deprecate the web_profiler.position option #23528

javiereguiluz opened this issue Jul 16, 2017 · 5 comments
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) WebProfilerBundle

Comments

@javiereguiluz
Copy link
Member

Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 3.4

This config option allows to display the debug toolbar at the top of the browser window instead of the default bottom position.

In the past we required this option because the old profiler displayed the toolbar at the top ... but the new profiler no longer needs it because it doesn't display the toolbar.

Although this may look as a small option, it requires some code to manage it:

  • The config code to define and validate the position option in web_profiler
  • The toolbarPosition argument is passed around in some profiler controllers
  • The CSS styles must define special styles when the toolbar is at the top
  • We even create a container parameter for this! web_profiler.debug_toolbar.position

Now that we don't need this option for ourselves, I propose to deprecate it in 3.4 and remove it in 4.0.

@javiereguiluz javiereguiluz added RFC RFC = Request For Comments (proposals about features that you want to be discussed) WebProfilerBundle labels Jul 16, 2017
@hhamon
Copy link
Contributor

hhamon commented Jul 16, 2017

I'm +1 too for deprecating it if we're sure it doesn't provide added value. If I remember well I had to move the toolbar at the top once because I was working on a page with an infinite scroll and the toolbar at the bottom was annoying in this case.

@xabbuh
Copy link
Member

xabbuh commented Jul 17, 2017

IIRC the use case @hhamon mentions was the reason why we rejected this idea in the past. However, how hard would it be to write custom CSS that does the same without the need for this option?

@javiereguiluz
Copy link
Member Author

javiereguiluz commented Jul 17, 2017

I can't understand why you can't use the web profiler in a page with "infinite scroll". This "feature" makes impossible to achieve the footer of the page, because the page always adds more contents ... but the debug toolbar is fixed at the bottom of the browser window. You can click on it without having to do any scroll. Moving the cursor over the debug toolbar won't make the page add more contents.

@nicolas-grekas
Copy link
Member

👍 if it removes code and help maintenance

@fabpot
Copy link
Member

fabpot commented Jul 28, 2017

IIRC, it was introduced to help display the toolbar on mobile, where having it on top is "better"/easier/worked (don't remember exactly).

fabpot added a commit that referenced this issue Sep 7, 2017
…iluz)

This PR was squashed before being merged into the 3.4 branch (closes #24080).

Discussion
----------

Deprecated the web_profiler.position option

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #23528
| License       | MIT
| Doc PR        | -

Related to #23728, which removes the feature for Symfony 4.0.

Commits
-------

53387b4 Deprecated the web_profiler.position option
@fabpot fabpot closed this as completed Sep 7, 2017
nicolas-grekas added a commit that referenced this issue Sep 9, 2017
…r.position option (javiereguiluz)

This PR was merged into the 4.0-dev branch.

Discussion
----------

[WebProfilerBundle] Removed the deprecated web_profiler.position option

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

This PR shows the things we could remove if we accept #23528. If it's accepted, I'd create the PR for 3.4 deprecating the feature instead of removing it.

Commits
-------

cafdf2b [WebProfilerBundle] Deprecated the web_profiler.position option
ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018
…viereguiluz)

This PR was squashed before being merged into the 3.4 branch (closes symfony#24080).

Discussion
----------

Deprecated the web_profiler.position option

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | symfony#23528
| License       | MIT
| Doc PR        | -

Related to symfony#23728, which removes the feature for Symfony 4.0.

Commits
-------

53387b4 Deprecated the web_profiler.position option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) WebProfilerBundle
Projects
None yet
Development

No branches or pull requests

5 participants