Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBundle] Give some love to exception pages#20525
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * @internal | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
second argument should be typehinted as array IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
7d249b0 to10b9db2Comparelinaori commentedNov 15, 2016
Are they opened by default? If so, can you add a limit of maybe 5 or 10 logs? I've had pages where there are 100+ logs and that will make it annoying to find the exception. |
nicolas-grekas commentedNov 15, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@iltar this PR is about fixing this page, ie make it work again; not about making it better. We could (and should) work on that for 3.3. But this one is a bug fix (placeholders are not replaced right now, and CSS needs a fix in the code block excerpt). |
10b9db2 tod8e9b6cComparefabpot commentedNov 15, 2016
I mis-read the code as well. 👍 |
linaori commentedNov 15, 2016
Ah my bad, I only saw new code 😆 |
| for ($i =max($line -$srcContext,1),$max =min($line +$srcContext,count($content));$i <=$max; ++$i) { | ||
| $lines[] ='<li'.($i ==$line ?'' :'').'><divid="line'.$i.'"></div><code>'.self::fixCodeMarkup($content[$i -1]).'</code></li>'; | ||
| $lines[] ='<li'.($i ==$line ?'' :'').'><aname="line'.$i.'"></a><code>'.self::fixCodeMarkup($content[$i -1]).'</code></li>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a[name] is not valid HTML5... what about<li><code></code></li> As anchor linking will work on any ID anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There can be only one specific id on a page, yet this is called several times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe add a static counter internally? To create something unique..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the target is not predictable, we're going to have a hard time linking to it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see. Maybe generate a hash of file+line.. or something, and only add a unique prefix for the 2nd and upcoming instances (ie. the first instance is predictable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a with name works, let's not create a pile of code to solve no actual issue...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Imo. it should produce valid HTML5 :).. but fair enough.
fabpot commentedNov 16, 2016
Thank you@nicolas-grekas. |
…ekas)This PR was merged into the 3.2-dev branch.Discussion----------[TwigBundle] Give some love to exception pages| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Before:After:Commits-------d8e9b6c [TwigBundle] Give some love to exception pages
Before:
After:
