-
Notifications
You must be signed in to change notification settings - Fork 174
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
[EEG Browser] Add ability to view annotations in event panel and overlaid on signal viewer #8235
[EEG Browser] Add ability to view annotations in event panel and overlaid on signal viewer #8235
Conversation
a55921a
to
37a9a93
Compare
@driusan Ready to be merged. |
modules/electrophysiology_browser/jsx/electrophysiologySessionView.js
Outdated
Show resolved
Hide resolved
e246870
to
a0b5be5
Compare
@maltheism, we need a JS guru to review this, do you have time to take a look? |
if (!(typeof text.json === 'string' | ||
|| text.json instanceof String)) return; | ||
return tsvParse( | ||
text.json.replace('trial_type', 'label')) | ||
.map(({ onset, duration, label }, i) => ({ |
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.
@laemtl npm isn't complaining that there's a space before onset and after label? Otherwise the code looks great.
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.
Eslint doesn't complain but I can fix it!
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 don't have a personal preference regarding the format of the code but I'm concerned why eslint isn't working? Today I encountered that before & after "space" issue for code I was writing for ccna.
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.
Maybe because it's a ts file and not a js file?
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.
Exactly, the ESLint config is not including the .tsx at the moment.
I can enable eslint a bit later once all the EEG PRs will be merged, since it will impact all the files.
Replaces #7828