-
Notifications
You must be signed in to change notification settings - Fork 175
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
[electrophysiology_browser] Add help directory and missing docs #6773
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.
Hi Karim, I did a first glance review.
Found 2 typos.
Please replace "scan" with "recording" everywhere because... EEG.
Please take a stab at describing briefly the relationships between the Downloads.
BIDS should be mentioned at least once.
modules/electrophysiology_browser/help/electrophysiology_browser.md
Outdated
Show resolved
Hide resolved
Co-authored-by: christinerogers <[email protected]>
5af9d64
to
28ed74e
Compare
28ed74e
to
e6b5ec4
Compare
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.
Hi Karim, thanks for these updates, could you take another pass to incorporate my points below -
Then, when ready (tomorrow) :
The last step for Help text is to paste a screenshot of how it actually renders. (Make sure both pages render well in the Help dropdown in your browser, and then paste a screenshot of each in this comment thread.)
Then ping me for re-review. thanks!
modules/electrophysiology_browser/help/electrophysiology_browser.md
Outdated
Show resolved
Hide resolved
|
||
Summary information about the candidate, the recording session and the hardware used is displayed. | ||
|
||
Recording files can be downloaded individually or as a compressed folder (*All Files* download): |
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.
Recording files can be downloaded individually or as a compressed folder (*All Files* download): | |
File packages can be downloaded containing just the recording signal, the events, or other metadata for this session - or all files compressed together (*All Files* download). These file packages are organized according to the EEG-BIDS standard, and available for download as follows: |
This suggestion/change because: the Recordings can't be downloaded "individually", it's the different types of files that are downloadable as packages.
Karim could you take another pass to look closer at the following?
- e.g I think "All Files" should be in the bulleted list (ordered according to the buttons you see)
- I'm glad you added link for BIDS; instead, let's link the EEG BIDS paper here, the URL is: https://www.nature.com/articles/s41597-019-0104-8
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 added "All Files" as part of the list. For the EEG-BIDS link, I created a footer to keep things neat in case more content was added to the file in the future.
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Here are the screenshots detailing the final changes: |
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.
Hi Karim - good work on this, suggestions below, and I think it's ready to come out of draft stage.
I can review again when ready.
modules/electrophysiology_browser/help/electrophysiology_browser.md
Outdated
Show resolved
Hide resolved
…logy_browser.md Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
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.
Thanks for your work on this Karim -- looks ready to go.
@driusan ready for final review.
Co-authored-by: christinerogers <[email protected]>
7ab0abd
to
f48cf2f
Compare
|
||
This module allows you to review and download EEG recordings and metadata. Downloads are organized according the EEG-BIDS standard. | ||
|
||
The selection filter allows you to narrow down candidate sessions. Clicking on any column headers in the data table to sort the table by that column. Individual sessions can be accessed by clicking on the *raw* (for raw datasets) or *all types* (for all data) links of one of the rows under the links column, which will redirect you to the *View Session* page. |
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.
The first 2 sentences violate the help style guide. (The second sentence is also grammatically incorrect.)
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.
My bad, I didn't notice the help style guide, I'll read it and format the files accordingly.
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.
In particular, I was referring to the part "Assume the user is tech-savvy enough to know about features that are intuitive or common across modern platforms, so you don’t need to repeat in every Help text".. the first example it gives it clicking on column headers to sort, but I think filtering in a selection filter also easily meets that criteria.
|
||
Files can be downloaded containing just the recording signal, the events, or other metadata, or all files compressed together, for the current session. These files are organized according to the EEG-BIDS standard<sup id="1">[1](#f1)</sup>, and are available for download as follows: | ||
|
||
- All files (tgz): a compressed folder containingscan all the recording data as well as metadata for the EEG scan session. |
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 is obviously a typo
|
||
Summary information about the candidate, the recording session and the hardware used is displayed. | ||
|
||
Files can be downloaded containing just the recording signal, the events, or other metadata, or all files compressed together, for the current session. These files are organized according to the EEG-BIDS standard<sup id="1">[1](#f1)</sup>, and are available for download as follows: |
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.
Just use a parenthesis to avoid mixing HTML and markdown.
Fixed typos, removed navigation explanation, changed "just" to "only" to makee it alex compliant, renamed view_sessions.md to sessions.md for loris to detect and display content.
just realized this PR thread never included the requested screenshots
@h-karim can you do one last check that the line-breaks, bullets, special chars etc render ok in the actual Help Dropdown in your Loris front-end, for both pages? So that any fixes can go in before 23.1 is released - thanks. |
@christinerogers Checked and added the screenshots to the description of the PR for reference. |
Brief summary of changes