Skip to content

[WebProfilerBundle] Update the logic that minimizes the toolbar #58380

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
Merged
merged 1 commit into from
Mar 29, 2025
Merged

[WebProfilerBundle] Update the logic that minimizes the toolbar #58380

merged 1 commit into from
Mar 29, 2025

Conversation

javiereguiluz
Copy link
Member

@javiereguiluz javiereguiluz commented Sep 24, 2024 •
edited
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

While working on fixing the accessibility issues of the toolbar (see #58366) I saw the following error reported:

Accesskey "D" is used more than once

This prevents the feature from working properly. So, in this PR I changed everything about how the toolbar is minimized:

  • Remove the "mini toolbar" (which is the button displayed when the toolbar is minimized)
  • Remove the .sf-toolbar-clearer element which is no longer needed
  • Transform the "hide" button into a "toggle" button to show/hide the toolbar
  • Update the show/hied logic to use CSS classes instead of just style: none/block

For end users nothing changes. It looks the same as before.

#SymfonyHackday

@javiereguiluz javiereguiluz added this to the 7.2 milestone Sep 24, 2024
stof
stof reviewed Sep 24, 2024
View reviewed changes
@javiereguiluz
Copy link
Member Author

@stof thanks a lot for the through review!

  • I fixed most of the issues that you pointed
  • About adding BC support for previous values, it's not worth it. The worst-case scenario is that someone had the toolbar closed an, after updating Symfony, the toolbar will be displayed. This is fine. Just click again on the close button.
  • Also, about close/open not being the same as show/hide ... it's true. But these changes don't modify the current behavior. The close/open buttons have been merged into a single toggle button which is always visible.

This is how it works on my local machine:

toolbar

@javiereguiluz javiereguiluz mentioned this pull request Sep 25, 2024
Merged
@stof
Copy link
Member

stof commented Sep 26, 2024

@javiereguiluz it does change the existing behavior. In 7.1, we have 3 different state:

  • hidden toolbar, used while the Ajax request loading it is running
  • closed toolbar
  • opened toolbar

Your PR shows a closed toolbar during loading (and if loading fails, this will be quite confusing)

@javiereguiluz
Copy link
Member Author

In my second commit I changed the default so the toolbar is visible by default. Would that be enough?

fabpot added a commit that referenced this pull request Sep 27, 2024
@fabpot
feature #58386 [WebProfilerBundle] Update the contents of the Config …
1687641 
…panel (javiereguiluz)

This PR was merged into the 7.2 branch.

Discussion
----------

[WebProfilerBundle] Update the contents of the Config panel

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -lain below instead -->
| License       | MIT

Similar to #58366 and #58380, this fixes an accessibility issue of the Web Debug Toolbar:

```
The "title" attribute can't be used in non-interactive elements
```

It now looks like this:

![image](https://github.com/user-attachments/assets/dff8d754-9661-40a7-81f9-5734f0ab36aa)

Commits
-------

9f8d2f9 [WebProfilerBundle] Update the contents of the Config panel
@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@javiereguiluz javiereguiluz removed the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Dec 7, 2024
@javiereguiluz
Copy link
Member Author

I've rebased this to Symfony 7.3 during the #SymfonyHackday of SymfonyCon 2024.

@OskarStark OskarStark requested a review from stof December 7, 2024 10:47
@fabpot fabpot force-pushed the toolbar_minimize branch from 14cb9d2 to 06ed516 Compare March 29, 2025 11:32
@fabpot
Copy link
Member

fabpot commented Mar 29, 2025

Thank you @javiereguiluz.

@fabpot fabpot merged commit cca60b4 into symfony:7.3 Mar 29, 2025
5 of 11 checks passed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature Status: Reviewed WebProfilerBundle
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
4 participants
@javiereguiluz @stof @fabpot @carsonbot