Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

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

Closed

Conversation

@sustmi
Copy link
Contributor

@sustmisustmi commentedApr 14, 2017•
edited
Loading

QA
Branch?3.3
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
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

Nek-, sstok, Koc, apfelbox, ogizanagi, Nicofuma, and adam-szaraniec reacted with heart emoji
@carsonbotcarsonbot added Status: Needs Review DXDX = Developer eXperience (anything that improves the experience of using Symfony) labelsApr 14, 2017
@sustmisustmi changed the title[Twig] [DX] Enhance exception page design[DX] [TwigBundle] Enhance exception page designApr 14, 2017
@sustmi
Copy link
ContributorAuthor

sustmi commentedApr 14, 2017•
edited
Loading

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

@sustmi
Copy link
ContributorAuthor

sustmi commentedApr 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
ContributorAuthor

sustmi commentedApr 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 commentedApr 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.

sstok, ro0NL, and Koc reacted with thumbs up emoji

@sustmi
Copy link
ContributorAuthor

sustmi commentedApr 15, 2017•
edited
Loading

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

10 lines5 lines
10-line-context5-line-context

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

@sustmisustmi changed the title[DX] [TwigBundle] Enhance exception page design[DX] [TwigBundle] Enhance the new exception page designApr 15, 2017
@sustmisustmiforce-pushed theenhance-exception-page-design branch from7c794b0 toc2225c4CompareApril 15, 2017 23:14
@sustmi
Copy link
ContributorAuthor

Rebased to the current master to rerun Travis CI.

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneApr 20, 2017
@nicolas-grekasnicolas-grekas modified the milestones:3.3,3.4Apr 28, 2017
@javiereguiluzjaviereguiluz self-assigned thisMay 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

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 commentedJun 1, 2017

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

apfelbox, ogizanagi, and sstok reacted with thumbs up emoji

@stofstof changed the base branch frommaster to3.4June 1, 2017 09:01
@stof
Copy link
Member

stof commentedJun 1, 2017

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

@ogizanagi
Copy link
Contributor

ogizanagi commentedJun 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. :)

yceruto, curry684, akerouanton, and jvasseur reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

@sustmi would you mind rebasing please?

@curry684
Copy link
Contributor

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.

@sustmisustmiforce-pushed theenhance-exception-page-design branch fromc2225c4 to6850170CompareJune 4, 2017 01:30
@sustmi
Copy link
ContributorAuthor

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

It was necessary to remove table layout and replace it with DIVs and SPANsbecause it was messing with width calculations.
@sustmisustmiforce-pushed theenhance-exception-page-design branch from6850170 to0173d22CompareJune 4, 2017 02:10
@sustmi
Copy link
ContributorAuthor

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 commitremove 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

You also need to change the target to 3.3.

@sustmisustmi changed the base branch from3.4 to3.3June 4, 2017 20:22
@sustmi
Copy link
ContributorAuthor

Oh, thanks for the notice. Done.

@fabpot
Copy link
Member

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

Copy link
Member

@javiereguiluzjaviereguiluz 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.

@fabpotfabpot closed thisJul 6, 2017
@fabpot
Copy link
Member

Thank you@sustmi.

fabpot added a commit that referenced this pull requestJul 6, 2017
…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 easilyCommits-------43212b9 [DX] [TwigBundle] Enhance the new exception page design
@fabpotfabpot mentioned this pull requestJul 17, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

Assignees

@javiereguiluzjaviereguiluz

Labels

DXDX = 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
[8]ページ先頭
©2009-2025 Movatter.jp