Skip to content

[WebProfilerBundle] Add collapsed sidebar on small screens #15818

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

Merged
merged 1 commit into from
Sep 26, 2015

Conversation

hason
Copy link
Contributor

@hason hason commented Sep 16, 2015

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

Desktop
Desktop

Mobile
Mobile Mobile touch

@hason hason changed the title [WebProfilerBundle] Add collapsed sidebar [WIP] [WebProfilerBundle] Add collapsed sidebar Sep 16, 2015
@javiereguiluz
Copy link
Member

I'm 👎 on this change too. I purposely removed this feature while the redesign.

My reasoning:

  1. The new sidebar is narrower than the old one, so the main contents enjoy more space.
  2. Collapsing the sidebar gives you just a bit more pixels for the content.
  3. Making the lines that contain text longer is not better but worse (as explained in the other pull request)
  4. Lately Symfony favors not adding new features and even removing some of them if possible. I think that this "collapsible sidebar" can be a "removable feature" and we could even get rid of the position config. option.

@hason in any case, keep in mind that my vote doesn't count for the merging or not of this PR, so you just have to convince the Symfony deciders.

<ul id="menu-profiler">
<li>
<a href="#" onclick="javascript: Sfjs.toggleClass(document.body, 'sidebar-collapse'); Sfjs.setPreference('sidebar/collapse', Sfjs.hasClass(document.body, 'sidebar-collapse')); return false;">
Copy link
Member

Choose a reason for hiding this comment

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

javascript: in the onclick handler is wrong. You are confusing onclick and special href

@stof
Copy link
Member

stof commented Sep 16, 2015

I'm not convinced either we need this. I can tell that I never collapsed the profiler menu (I collapse the toolbar sometimes, for instance when it hides some of my own position: fixed content, but not the menu)

@stloyd
Copy link
Contributor

stloyd commented Sep 16, 2015

👎 for collapsing using javascript,
👍 for collapsing on small screens with expanding on click/on hover.

@hason hason changed the title [WIP] [WebProfilerBundle] Add collapsed sidebar [WIP] [WebProfilerBundle] Add collapsed sidebar on small screens Sep 17, 2015
@hason hason changed the title [WIP] [WebProfilerBundle] Add collapsed sidebar on small screens [WebProfilerBundle] Add collapsed sidebar on small screens Sep 17, 2015
@hason
Copy link
Contributor Author

hason commented Sep 17, 2015

The collapsed sidebar is enabled only on small screens.

@hason hason force-pushed the sidebar branch 2 times, most recently from 739d70a to b0bb382 Compare September 17, 2015 07:51
@javiereguiluz
Copy link
Member

Now that the behavior of this proposed feature has changed, I change my vote too for 👍

There are very small alignment issues:

  • When collapsed, the menu icon doesn't appear centered horizontally as the other icons.
  • When expanded, the icon of the Search button should be centered.
  • When expanded, the menu icon should be vertically centered with the "Latest Profiles" and the Search buttons.

@hason
Copy link
Contributor Author

hason commented Sep 17, 2015

@javiereguiluz I tried to correct alignment issues, but I'm not a webmaster. Could you review it, please?

@fabpot
Copy link
Member

fabpot commented Sep 26, 2015

Thank you @hason.

@fabpot fabpot merged commit 184d4f2 into symfony:2.8 Sep 26, 2015
fabpot added a commit that referenced this pull request Sep 26, 2015
…eens (hason)

This PR was merged into the 2.8 branch.

Discussion
----------

[WebProfilerBundle] Add collapsed sidebar on small screens

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

Desktop
![Desktop](https://cloud.githubusercontent.com/assets/288535/9927840/1e4eb7fc-5d22-11e5-9833-f1431ed2855d.png)

Mobile
![Mobile](https://cloud.githubusercontent.com/assets/288535/9927847/23a5c5c4-5d22-11e5-928a-87b167a079b5.png) ![Mobile touch](https://cloud.githubusercontent.com/assets/288535/9927851/2bb800e2-5d22-11e5-9566-c74a23eb9cb5.png)

Commits
-------

184d4f2 [WebProfilerBundle] Added collapsed sidebar on small screens
@hason hason deleted the sidebar branch October 8, 2015 08:20
@fabpot fabpot mentioned this pull request Nov 16, 2015
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
…all screens (hason)

This PR was merged into the 2.8 branch.

Discussion
----------

[WebProfilerBundle] Add collapsed sidebar on small screens

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

Desktop
![Desktop](https://cloud.githubusercontent.com/assets/288535/9927840/1e4eb7fc-5d22-11e5-9833-f1431ed2855d.png)

Mobile
![Mobile](https://cloud.githubusercontent.com/assets/288535/9927847/23a5c5c4-5d22-11e5-928a-87b167a079b5.png) ![Mobile touch](https://cloud.githubusercontent.com/assets/288535/9927851/2bb800e2-5d22-11e5-9566-c74a23eb9cb5.png)

Commits
-------

184d4f2 [WebProfilerBundle] Added collapsed sidebar on small screens
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants