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

Appearance settings

[WebProfilerBundle] [WIP] Add help in Web Profiler #21046

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

Closed
wants to merge 4 commits into from

Conversation

maidmaid
Copy link
Contributor

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

Hello,

Symfony is a truly ideal framework to teach young future web developer to create a complete and robust web application, thanks to the excellent quality of its documentation.

Several links of this documentation are already present directly in the source code, making learning and understanding of Symfony easier. In the same idea, this PR proposes to add links of documentation in the Web Profiler, to allow a better pedagogical experiment.

The next first example adds cookie doc link:

screenshot from 2016-12-24 22-21-10

WDYT?

@maidmaid maidmaid changed the title [Web Profiler] [WIP] Add help in Web Profiler [WebProfilerBundle] [WIP] Add help in Web Profiler Dec 24, 2016
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

I really really like this idea!

(and enjoy your christmas! 🎄)

@@ -46,6 +46,7 @@ public function getConfigTreeBuilder()
->end()
->booleanNode('intercept_redirects')->defaultFalse()->end()
->scalarNode('excluded_ajax_paths')->defaultValue('^/(app(_[\\w]+)?\\.php/)?_wdt')->end()
->booleanNode('help')->defaultFalse()->end()
Copy link
Member

Choose a reason for hiding this comment

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

As Symfony already has lots of config options, it's often tried to avoid adding many more options. I think this is a case where a config option is not required. Always enabling help will only add some minor question mark icons, which aren't that distracting (and maybe even helpfull for everyone). Always enabling it will also make code a lot easier (avoiding many if statements).

@@ -72,6 +76,7 @@ public function getFunctions()
return array(
new \Twig_SimpleFunction('profiler_dump', $profilerDump, array('is_safe' => array('html'), 'needs_environment' => true)),
new \Twig_SimpleFunction('profiler_dump_log', array($this, 'dumpLog'), array('is_safe' => array('html'), 'needs_environment' => true)),
new \Twig_SimpleFunction('help', array($this, 'help'), array('is_safe' => array('html'))),
Copy link
Member

Choose a reason for hiding this comment

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

I would propose to call this help_attrs or the like, to cover the usage of the method more (calling it attrs makes it very clear that this has to be executed inside a tag).

.help {
display: inline-block;
text-align: center;
background-color: #6da581;
Copy link
Member

Choose a reason for hiding this comment

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

I propose to make the color a bit lighter (less contrasting with the background, e.g. a light grey). Help icons shouldn't get attention when looking the page, they should only be visible when one needs it.

I'm afraid using the highlight/primary color will make it too distracting.

@wouterj
Copy link
Member

wouterj commented Dec 25, 2016

status: needs work

@linaori
Copy link
Contributor

linaori commented Dec 25, 2016

I'm all for this idea!

@ro0NL
Copy link
Contributor

ro0NL commented Dec 25, 2016

👍 we should keep a single resource though containing all references (think maintenance).

And perhaps we should just provide a list of links (imo. there can be more relevent references then doc+api).

# help.yml
cookies:
  - '/some/ref'
  - '/other-ref'

# etc.

Also 👍 for making it configurable :)

@maidmaid
Copy link
Contributor Author

maidmaid commented Dec 25, 2016

I have added help.yml containing all references as @ro0NL suggested, removed help config, refactored help_attr twig function and improved design like this:

screenshot from 2016-12-26 00-58-13

@javiereguiluz
Copy link
Member

I think I like the idea ... but the implementation looks overkill to me. Why can't we just simply put the links in the HTML templates instead of using a YAML config file + Twig functions? (but please, don't make any change yet, this is just a comment)

@fabpot
Copy link
Member

fabpot commented Dec 26, 2016

Regarding the implementation: I agree with @javiereguiluz

Regarding the feature: I'm not sure that people looking at cookies in the web profiler expect to get documentation or API docs from there. Let's see what other @symfony/deciders think about it.

@maidmaid
Copy link
Contributor Author

maidmaid commented Dec 26, 2016

@javiereguiluz the Twig function manages the Symfony version of the project that is included in the documentation URL. These dynamic URL can not be defined directly in templates.

@fabpot Yes, the example of cookies is not the most relevant, but for example links of documentation in the Security section will be very useful.

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 26, 2016
@ro0NL
Copy link
Contributor

ro0NL commented Dec 26, 2016

Should we consider having Sfjs.set/getPreference to make this opt-in/out (rather easily)? And perhaps find a clean way to have some simple panel to actually control/expose preferences?

I proposed the YAML file.. and it could indeed be overkill (not sure how many links we expect to end up around templates?). Perhaps a static array in code is better?

@fabpot
Copy link
Member

fabpot commented Dec 26, 2016

As you know, we try very hard to not create new option/configuration when not strictly needed. This is a great example of something that should not be configured. If we think it's interesting, it should always be there (and it's not that disturbing anyway). But before going further, I would like to get a list of links we could add and where. I'm still not convinced that this is the best place to add links to the docs.

@ro0NL
Copy link
Contributor

ro0NL commented Dec 26, 2016

This is a great example of something that should not be configured.

Agree. I think the no. of question icons shown on the same page is crucial here.

With the latest design (sorry) i think after a few days the grey dots would start annoy me. So the design should be really slick here (smaller icons imo.).., but that's matter of preference (<punch line here>).

@dunglas
Copy link
Member

dunglas commented Dec 26, 2016

I like the idea of making it easy for newcomers to know where to find the relevant documentation. IMO, having the link to the article is enough, the API doc will be displayed directly in the IDE if the user uses a good one.

@ogizanagi
Copy link
Contributor

@ro0NL : If you fear to be annoyed by these helping dots, I'd rather invite you to use something like Stylish and a custom css rule instead of making it configurable 😄
(However the dots seem way too big on the current screenshot IMHO 😅)

@ro0NL
Copy link
Contributor

ro0NL commented Dec 26, 2016

@ogizanagi eventually yes. But if people start doing that, maybe the design is bad. Note im not saying there's a holy grail or so which works for everbody.

Im not so sure about API links, they dont seem relevant. And wouldnt it be nicer to have 1 single help icon per page? I.e on top?

@stof
Copy link
Member

stof commented Dec 26, 2016

I agree with @javiereguiluz and @fabpot about this system being overkill.

And I think that links to the doc are more useful than links to the API doc. I would not link to the API doc from the profiler.

@@ -72,6 +79,7 @@ public function getFunctions()
return array(
new \Twig_SimpleFunction('profiler_dump', $profilerDump, array('is_safe' => array('html'), 'needs_environment' => true)),
new \Twig_SimpleFunction('profiler_dump_log', array($this, 'dumpLog'), array('is_safe' => array('html'), 'needs_environment' => true)),
new \Twig_SimpleFunction('help_attrs', array($this, 'help'), array('is_safe' => array('html'))),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be prefixed with profiler_ to prevent conflicts with other extensions.

@maidmaid
Copy link
Contributor Author

About @fabpot quote:

But before going further, I would like to get a list of links we could add and where.

Here, my list of links proposal:

Main Title Symfony Profiler (on top right)

Panel Request / Response

Panel Performance

  • Title Performance (missing title): Doc

Panel Logs

Panel Forms

For each fields:

  • Title <name field>: doc of form type (example with TextType type form: Doc)
  • Section Default Data: Doc
  • Section Submitted Data: Doc
  • Section Passed Options and Resolved Options:
    • for each option, doc of option (example with label option for TextType form type: Doc)
    • for each constraint option, doc of constraint (example with NotBlank constraint: Doc)

Panel Exception

  • Title Exceptions: Doc

Panel Events

  • Title Event Dispatcher: Doc, Component, Tools
  • Event kernel.request: Doc
  • Event kernel.controller: Doc
  • Event kernel.view: Doc
  • Event kernel.response: Doc
  • Event kernel.finish_request: Doc
  • Event kernel.terminate: Doc
  • Event kernel.exception: Doc
  • Event console.command: Doc
  • Event console.exception: Doc
  • Event console.terminate: Doc

Panel Routing

Panel Translation

Panel Security

Section Security Token:

  • Box Authenticated: Doc
  • Property Roles: Doc
  • Property Inherited Roles: Doc

Section Security Firewall:

  • Section Security Firewall: Doc
  • Key provider: Doc
  • Key entry_point: Doc
  • Key user_checker: Doc
  • Key access_denied_handler: Doc
  • Key listeners: Doc

Section Security Voters:

  • Section Security Voters: Doc
  • Box Strategy: Doc
  • Voter class Symfony\Component\Security\Core\Authorization\Voter\RoleVoter: Doc
  • Voter class Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter: Doc

Panel Twig

Panel Doctrine

  • Title Doctrine (missing title): Doc, Config, Tools, Doc Doctrine
  • Section Queries: Doc
  • Sections Database Connections and Entity Managers: Doc
  • Section Second Level Cache: Doc
  • Section Entities Mapping: Doc

Panel E-mails

Panel Debug

Panel Configuration

  • Box Environment: Doc
  • Box Application name: Doc
  • Section Enabled Bundles: Doc

@tjaari
Copy link

tjaari commented Dec 29, 2016

Nice addition 👍

@fabpot
Copy link
Member

fabpot commented Dec 30, 2016

I still think this is not the right place for such links. But that's just my opinion of course. @symfony/deciders WDYT?

@fabpot
Copy link
Member

fabpot commented Dec 30, 2016

By the way, #21081 seems like a much better idea.

@fabpot
Copy link
Member

fabpot commented Feb 16, 2017

Closing as I haven't changed my mind. That's not the right place.

@fabpot fabpot closed this Feb 16, 2017
@nicolas-grekas nicolas-grekas modified the milestone: 3.x Mar 24, 2017
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.

Morty Proxy This is a proxified and sanitized view of the page, visit original site.