-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
MRG: Revamp HTML reprs of Raw
, Epochs
, Evoked
, and Info
#12583
Conversation
… in design and in content
…ne-python into evoked-html-repr
you're aligning to the minimum info by dropping number of channels and time points. I find it useful to know the duration and how many channels are available. |
@agramfort Are you suggesting to add number of samples and channels to the Epochs repr instead, then? I could do that, no problem |
yes I would have done that if you push for consistency.
… Message ID: ***@***.***>
|
Ok i'll take a look! |
Evoked
HTML repr to closer match the one from Epochs
, both in design and in contentRaw
, Epochs
, Evoked
to be more consistent
I'm generally quite happy with the improved consistency. Thanks! Nitpicks:
|
I was worried this could be confused with "sampling points" on the time axis … I had first thought of labeling it "digitized had shape and sensor positions", but it's too long. I'm okay with changing it back to "digitized points", but it's not my favorite – maybe if we brainstorm a little, we can come up with a better idea?
Yes, it was there before and I'm actually not sure if it's really needed?
Good idea, I can do that! |
@drammock Another thing we may need to address – these tables are quite long now, and I haven't added collapsible sections like we have for |
Raw
, Epochs
, Evoked
to be more consistentRaw
, Epochs
, Evoked
to be more consistent
+1 for |
…voked-html-repr
I think this is ready to be merged. It was quite challenging to build something that works and looks okay-ish in VS Code, Jupyter, and in Report. For example, I wanted to use the Popover API to create popups, but VS Code would render the Python Console input widgets above my popover content, while Jupyter wouldn't. Problems such as these. Anyway, here are some examples: VS Code Python Interactive, light themeScreen.Recording.2024-05-19.at.09.14.44.movVS Code Python Interactive, dark themeReport, light themeScreen.Recording.2024-05-19.at.09.57.58.movReport, dark themeJupyter Lab, light themeJupyter Lab, dark themeAs you can see, there's a problem with dark theme support in Report and Jupyter Lab (but not in VS Code Python Interactive), as the collapse/uncollapse caret is simply changed to a white color, which of course only works if the background is dark. I think for now this is good enough and I will try to improve dark theme support in some follow-up PRs; I've been wanting to add a dark theme to Report for a while now anyway. Also the examples demonstrate that now, you can click on the entire section header row (not just on the carets) to collapse/uncollapse a section. Rendered tutorial: https://output.circle-artifacts.com/output/job/d78b2fa9-df94-4d24-ad32-1b2806c11212/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data I snuck in a fix fox #12553 as well |
This comment was marked as resolved.
This comment was marked as resolved.
I realized there's still some issues with the text alignment in Jupyter Lab, need to fix those first. |
Raw
, Epochs
, Evoked
, and Info
Raw
, Epochs
, Evoked
, and Info
@mscheltienne feel free to review/merge if you're happy, or if you don't have time I can look |
I'm away until Thursday, I can look then. |
@larsoner If you want, go ahead and merge; and I can address comments from @mscheltienne in a follow-up PR (I expect we'll have to do a few of those once we start using the new code) |
let's get this in, I agree that follow-up PRs are fine |
Looks good to me, the only comment I have is on the pop-up you get when clicking on the number of channels (good or bad); on VSCode I don't find the notification very easy to use. But that's a very minor comment for a great refactor! |
This aims to address a concern @mscheltienne voiced at mne-tools#12583 (comment) Now, the channel names can already be viewed by simply hovering over the number of good or bad channels. The appearing overlay still lets the user know that by clicking, they can open the channel names in a popup window.
For example, the
Evoked
repr contained the number of channels, but failed to subtract or report any bads.Also I believe reporting the number of sampling points is quite pointless , so I removed that as well.
The table layout wasn't up-to-date either.
main
(striped layout only added by VS Code, actually not present in the tables we produce!)This PR:
For comparison,
Epochs
:MWE:
This one also
closes #12553