Skip to content
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

Add help page #120

Merged
merged 6 commits into from
Nov 28, 2017
Merged

Add help page #120

merged 6 commits into from
Nov 28, 2017

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Nov 6, 2017

Per feedback on this PR, help for the keyboard shortcuts is added via a modal triggered from a button.

screen shot 2017-11-16 at 3 25 07 pm

screen shot 2017-11-16 at 3 25 10 pm

Edit — Info below this header is outdated

Add help documentation to the UI.

Currently, it has a few links and covers the keyboard shortcuts for the trace timeline.

One of the issues I ran into with this change was loading markdown files. It's not well supported in create-react-app. The recommended approach is to fetch the markdown at runtime and parse it in the browser (which is absurd):

There's no longer a way to directly import Markdown files. In case anyone finds their way here through Google, you could always use fetch() to accomplish this asynchronously:

// return the URL, e.g. /static/media/changelog.1fb9caf0.md
import myMarkdownFile from '../../../changelog.md';

fetch(myMarkdownFile)
 .then(response => response.text())
 .then(text => {
   // logs a string of Markdown content
   console.log(text);
 });

However, outside the scope of condoned approaches is inline webpack loader configuration:

facebook/create-react-app#1944 (comment)

[...]

/* eslint import/no-webpack-loader-syntax: off */
import QuickStart from '!raw-loader!./Sections/QuickStart.md';

Inline webpack loader configuration is considered "not permitted" in create-react-app.

But, it gets the job done and can be changed to some other approach, in the future, if that becomes necessary.

IMO, even with these trade-offs, it is still preferable to use markdown over regular HTML for docs.

Definitely open to feedback on this approach.

@yurishkuro
Copy link
Member

how is this help page going to be invoked? I don't think we want to start keeping real documentation in the UI project, that's what readthedocs is for. For the key bindings I was thinking some help button in the Trace View that shows a popup - markdown might be an overkill for that.

Copy link
Member

@saminzadeh saminzadeh left a comment

Choose a reason for hiding this comment

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

However, outside the scope of condoned approaches is inline webpack loader configuration:

I'm a bit impartial of getting into the webpack internals through CRA. I think it might be better to wait and see what they do with static assets like markdown.

Personally, I don't think its too crazy to fetch the help file when requested (think of it as code splitting which reduces the bundle size anyways). Some alternatives would be using a JS file and export markdown directly.

This document is a work in progress. To request additional documentation of the Jaeger UI, please create a [documentation ticket](https://github.com/jaegertracing/jaeger-ui/issues/new?labels=documentation).


## Keyboard Shortcuts
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in a modal for the Trace page, since its specific to that. A little tooltip can open it up

@tiffon
Copy link
Member Author

tiffon commented Nov 6, 2017

I prefer the help button and the page, but a modal sounds fine.

@saminzadeh, @yurishkuro - any suggestions on what invokes the modal?

To answer your question, @yurishkuro, I added a "Help" button to the top menu (which I'll now take out).

@saminzadeh
Copy link
Member

I was thinking a little Question mark icon on the view trace page. Something similar to codepen:

image

@tiffon
Copy link
Member Author

tiffon commented Nov 7, 2017

Great. I was thinking something similar, but with the consensus approach being so far removed from my initial approach, seemed prudent to invite input, preemptively.

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 68.948% when pulling 3ad7935 on add-help-page into df12ead on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 68.948% when pulling 3ad7935 on add-help-page into df12ead on master.

Copy link
Member

@saminzadeh saminzadeh left a comment

Choose a reason for hiding this comment

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

Nice, the screenshot looks great

@tiffon tiffon merged commit 826dfda into master Nov 28, 2017
@yurishkuro yurishkuro deleted the add-help-page branch January 29, 2020 15:11
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Add help page

Signed-off-by: Joe Farro <joef@uber.com>

* Revert adding help as a separate page

Signed-off-by: Joe Farro <joef@uber.com>

* Trace detail keyboard shortcuts help as a modal

Signed-off-by: Joe Farro <joef@uber.com>

* Misc typo

Signed-off-by: Joe Farro <joef@uber.com>

* Misc cleanup

Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants