Skip to content

[VarDumper] Fix blank strings display #59390

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
Jan 9, 2025

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jan 7, 2025

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #57980
License MIT

Because sf-dump-ellipsis spans needed a text-ellipsis, their overflowing content was cut using overflow: hidden. As it required their inner display type to be block, this broke the alignment with the ellipsis’ “tail”. This was fixed by #53147 by making every dump’s spans inline-flex.

This change made sf-dump-ellipsisdisplay, max-width and vertical-align properties useless so this PR removes them, as well as a duplicated overflow one.

Now, inline-flex elements’ content becomes flex items, which caused #57980 because

if the entire sequence of [a flex item’s] text runs contains only white space […] it is […] not rendered

https://www.w3.org/TR/css-flexbox-1/#flex-items

Instead of making every dump’s spans inline-flex, this PR targets a new sf-dump-ellipsization class added to sf-dump-ellipsis’ parents.

It also wraps ellipsis tails with elements bearing the sf-dump-ellipsis-tail class so that we can prevent them to shrink:

Before:

After:

@carsonbot carsonbot added this to the 6.4 milestone Jan 7, 2025
@MatTheCat MatTheCat force-pushed the ticket_57980 branch 5 times, most recently from 7b693f2 to 4a43bad Compare January 7, 2025 11:07
"<pre class=sf-dump id=sf-dump data-indent-pad=\" \"><span class=sf-dump-num>123</span>\n</pre><script>Sfdump(\"sf-dump\")</script>\n"
."<pre class=sf-dump id=sf-dump data-indent-pad=\" \"><span class=sf-dump-num>456</span>\n</pre><script>Sfdump(\"sf-dump\")</script>\n",
"<pre class=sf-dump id=sf-dump data-indent-pad=\" \"><span class=\"sf-dump-num\">123</span>\n</pre><script>Sfdump(\"sf-dump\")</script>\n"
."<pre class=sf-dump id=sf-dump data-indent-pad=\" \"><span class=\"sf-dump-num\">456</span>\n</pre><script>Sfdump(\"sf-dump\")</script>\n",
Copy link
Member

Choose a reason for hiding this comment

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

this change breaks low-deps
better preserve the previous output when possible to not have to change version constraints
doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could quote the class attribute only if there are two indeed (would feel weird though 😅)

Copy link
Member

Choose a reason for hiding this comment

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

that'd help fix tests (they're still red :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn’t high and low-deps tests eventually pass when the code is released?

Anyways, I limited the changes to ellipsization so that other components’ tests are not impacted 🟢

@nicolas-grekas
Copy link
Member

Thank you @MatTheCat.

@nicolas-grekas nicolas-grekas merged commit 89ba0e5 into symfony:6.4 Jan 9, 2025
11 checks passed
@MatTheCat MatTheCat deleted the ticket_57980 branch January 9, 2025 15:32
This was referenced Jan 29, 2025
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.

3 participants