8000 [DX] [TwigBundle] Enhance the new exception page design by sustmi · Pull Request #22439 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX] [TwigBundle] Enhance the new exception page design #22439

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
Closed
wants to merge 8 commits into from
Closed

[DX] [TwigBundle] Enhance the new exception page design #22439

wants to merge 8 commits into from

Conversation

sustmi
Copy link
Contributor
@sustmi sustmi commented Apr 14, 2017 •
edited
Loading
Q A
Branch? 3.3
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I got motivated by recent PR #20951 and redesigned the exception page even more.

Compare before: before
with after: after
(Ignore the the "sf" toolbar icon in the middle of the page. This is how Firefox does full-page screenshots.)

The most noticeable changes:

  • removed double line spacing (it just does not feel natural)
  • file context increased from 3 to 10 lines (it helps me to better orientate in the code)
  • added horizontal scrollbar for the cases when the code is wider
  • the highlighted line color is more saturated in order to be noticed easily

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Apr 14, 2017
@sustmi sustmi changed the title [Twig] [DX] Enhance exception page design [DX] [TwigBundle] Enhance exception page design Apr 14, 2017
@sustmi
Copy link
Contributor Author
sustmi commented Apr 14, 2017 •
edited
Loading

Wait, I found that the problem with wrapping long lines shown here: #20951 (comment) still happens.

@sustmi
Copy link
Contributor Author
sustmi commented Apr 14, 2017 •
edited
Loading

Wrapping is fixed in the new commit.

@robfrawley
Copy link
Contributor

Personally, I preferred the less dense "double line" of the original.

@sustmi
Copy link
Contributor Author
sustmi commented Apr 14, 2017 •
edited
Loading

@robfrawley, do you think that with single line spacing the code is harder to read?
I guess that single line spacing is the most common setting in IDEs and editors (I have never seen anybody use double line spacing for PHP) and therefore I consider single line spacing more natural for reading PHP code (it should create less cognitive load).

@curry684
Copy link
Contributor
curry684 commented Apr 14, 2017 •
edited
Loading

file context increased from 3 to 10 lines (it helps me to better orientate in the code)

👎 on that one as it then dominates the average developer's screen completely. And you can just middle/right click the filename above the snippet to see the full scrollable context if you really need it.

Totally down with bringing the line spacing down though, just tried it and yes it reads more like actual code. And it does give room to increase the context to 5 without sacrificing call stack.

@sustmi
Copy link
Contributor 8000 Author
sustmi commented Apr 15, 2017 •
edited
Loading

OK, I lowered the file excerpt line context to 5. You can see the difference here:

10 lines 5 lines
10-line-context 5-line-context

Now it has approximately the same height as the 3-line context with double line spacing.
What do you think?

@sustmi sustmi changed the title [DX] [TwigBundle] Enhance exception page design [DX] [TwigBundle] Enhance the new exception page design Apr 15, 2017
@sustmi sustmi force-pushed the enhance-exception-page-design branch from 7c794b0 to c2225c4 Compare April 15, 2017 23:14
@sustmi
Copy link
Contributor Author
sustmi commented Apr 15, 2017

Rebased to the current master to rerun Travis CI.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Apr 20, 2017
@nicolas-grekas nicolas-grekas modified the milestones: 3.3, 3.4 Apr 28, 2017
@ro0NL ro0NL mentioned this pull request May 30, 2017
Merged
@javiereguiluz javiereguiluz self-assigned this May 31, 2017
@javiereguiluz
Copy link
Member

I'm sorry I didn't review this on time for 3.3.0 ... I even thought this was merged! There are some things that I'm not sure about ... but others that I like. Tomorrow I'll add more details. Thanks!

@apfelbox
Copy link
Contributor
apfelbox commented Jun 1, 2017

About the issues with the line count in the code excerpts… how about the way Github does this?

2017-06-01 at 10 54

Or would this be overkill?

@stof
Copy link
Member
stof commented Jun 1, 2017

@apfelbox IMO, this is overkill. You have access to the full file anyway if you want.

@stof stof changed the base branch from master to 3.4 June 1, 2017 09:01
@stof
Copy link
Member
stof commented Jun 1, 2017

@sustmi can you rebase this on top of the Symfony 3.4 branch to fix conflicts ?

@ogizanagi
Copy link
Contributor
ogizanagi commented Jun 2, 2017 •
edited
Loading

As I said in #22971 (comment), I'd love to see this merged in 3.3 as a bug fix rather than in 3.4. There is no DX nor feature to me, just UI/UX improvements and fixes for minor but annoying behavior issues. :)

@nicolas-grekas
Copy link
Member

@sustmi would you mind rebasing please?

@curry684
Copy link
Contributor
curry684 commented Jun 2, 2017

I second the 3.3.1 vote. It's not a feature, just an improvement on 3.3-introduced behavior. DX/UX improvements have always been allowed in patch releases, no reason to postpone it.

@sustmi sustmi force-pushed the enhance-exception-page-design branch from c2225c4 to 6850170 Compare June 4, 2017 01:30
@sustmi
Copy link
Contributor Author
sustmi commented Jun 4, 2017

I rebased the PR to 3.4 as @stof adviced. Is that all right?

sustmi added 2 commits June 4, 2017 03:52
@sustmi
enable horizontal scrollbar in file excerpt
db8ba80 
It was necessary to remove table layout and replace it with DIVs and SPANs
because it was messing with width calculations.
@sustmi
increase context in file excerpt from 3 to 10 lines
8c9c35c
@sustmi sustmi force-pushed the enhance-exception-page-design branch from 6850170 to 0173d22 Compare June 4, 2017 02:10
@sustmi
Copy link
Contributor Author
sustmi commented Jun 4, 2017

I just noticed the discussion in #22971 (comment) so I guess you actually wanted me to rebase it to 3.3.
So I rebased it to 3.3 and I also removed commit remove double line spacing to make it more compact and similar to IDEs as @ogizanagi already changed line-spacing from 10px to 5px in #22971 and @nicolas-grekas seemed to like it in #22971 (comment) as it is consistent with GitHub look.

@fabpot
Copy link
Member
fabpot commented Jun 4, 2017

You also need to change the target to 3.3.

@sustmi sustmi changed the base branch from 3.4 to 3.3 June 4, 2017 20:22
@sustmi
Copy link
Contributor Author
sustmi commented Jun 4, 2017

Oh, thanks for the notice. Done.

@fabpot
Copy link
Member
fabpot commented Jun 9, 2017

@javiereguiluz Is it ready to merge or do you need more time to review?

javiereguiluz
javiereguiluz approved these changes Jul 6, 2017
View reviewed changes
Copy link
Member
@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

👍 I've reviewed everything again and it looks good to me.

In your screenshots some things look different ... for example some border are not visible:

border-exception

... but I've reviewed the CSS styles and they look OK, so there must be some problem with the screenshot.

@fabpot fabpot closed this Jul 6, 2017
@fabpot
Copy link
Member
fabpot commented Jul 6, 2017

Thank you @sustmi.

fabpot added a commit that referenced this pull request Jul 6, 2017
@fabpot
bug #22439 [DX] [TwigBundle] Enhance the new exception page design (s…
626e987 
…ustmi)

This PR was squashed before being merged into the 3.3 branch (closes #22439).

Discussion
----------

[DX] [TwigBundle] Enhance the new exception page design

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

- [x] Fix the problem with wrapping wide lines (#20951 (comment))

I got motivated by recent PR #20951 and redesigned the exception page even more.

Compare before: ![before](https://cloud.githubusercontent.com/assets/885946/25052220/0756b74e-2151-11e7-98b6-c99fd9eaabec.png)
with after: ![after](https://cloud.githubusercontent.com/assets/885946/25052225/0c76581a-2151-11e7-96ff-eb502ee8e97b.png)
(Ignore the the "sf" toolbar icon in the middle of the page. This is how Firefox does full-page screenshots.)

The most noticeable changes:
- removed double line spacing (it just does not feel natural)
- file context increased from 3 to 10 lines (it helps me to better orientate in the code)
- added horizontal scrollbar for the cases when the code is wider
- the highlighted line color is more saturated in order to be noticed easily

Commits
-------

43212b9 [DX] [TwigBundle] Enhance the new exception page design
@fabpot fabpot mentioned this pull request Jul 17, 2017
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

@javiereguiluz javiereguiluz javiereguiluz approved these changes

Assignees

@javiereguiluz javiereguiluz

Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
10 participants
@sustmi @robfrawley @curry684 @javiereguiluz @apfelbox @stof @ogizanagi @nicolas-grekas @fabpot @carsonbot
0