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.

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

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

Open
wants to merge 3 commits into
base: 7.2
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

javiereguiluz
Copy link
Member

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.

@javiereguiluz javiereguiluz added this to the 7.2 milestone Sep 24, 2024
stof
stof reviewed Sep 24, 2024
View reviewed changes
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.html.twig Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Outdated Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Outdated Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Outdated Show resolved Hide resolved
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_js.html.twig Outdated Show resolved Hide resolved
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Needs Review WebProfilerBundle
Projects
None yet
Milestone
7.2
Development

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