-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
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 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() |
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.
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'))), |
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 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; |
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 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.
status: needs work |
I'm all for this idea! |
👍 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 :) |
I have added |
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) |
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. |
@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. |
Should we consider having 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? |
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. |
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>). |
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 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? |
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'))), |
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.
This should be prefixed with profiler_
to prevent conflicts with other extensions.
About @fabpot quote:
Here, my list of links proposal: Main Title
|
Nice addition 👍 |
I still think this is not the right place for such links. But that's just my opinion of course. @symfony/deciders WDYT? |
By the way, #21081 seems like a much better idea. |
Closing as I haven't changed my mind. That's not the right place. |
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:
WDYT?