8000 MRG, DOC: Improve styling using classes by larsoner · Pull Request #7174 · mne-tools/mne-python · GitHub
[go: up one dir, main page]
Skip to content

MRG, DOC: Improve styling using classes #7174

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 6, 2020
Merged

MRG, DOC: Improve styling using classes #7174

merged 3 commits into from
Jan 6, 2020

Conversation

larsoner
Copy link
Member
@larsoner larsoner commented Jan 5, 2020 •
edited
Loading

Proof of concept for sphinx-gallery/sphinx-gallery#581. It makes MNE links in code blocks bold, and leaves the other ones (Python, Matplotlib, etc.) as the standard weight.

It also removes reset-syntax.css in favor of just having our CSS all in style.css.

WIP until the SG PR is merged, then I'll remove the circleci changes.

@codecov
Copy link
codecov bot commented Jan 5, 2020 •
edited
Loading

Codecov Report

Merging #7174 into master will decrease coverage by 1.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7174      +/-   ##
==========================================
- Coverage   89.76%   88.48%   -1.28%     
==========================================
  Files         444      444              
  Lines       79384    79383       -1     
  Branches    12692    12692              
==========================================
- Hits        71258    70243    -1015     
- Misses       5344     6353    +1009     
- Partials     2782     2787       +5

@larsoner larsoner changed the title WIP, DOC: Improve styling using classes MRG, DOC: Improve styling using classes Jan 6, 2020
@larsoner
Copy link
Member Author
larsoner commented Jan 6, 2020

Okay this one is good to go now that the SG PR is merged:

https://17414-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/intro/plot_10_overview.html

@drammock can you look and push commits if you don't like the style changes?

@drammock
Copy link
Member
drammock commented Jan 6, 2020

I'm fine with this style change, I think it looks fine. But I have two questions:

  1. is it a bug that sometimes variable names get linked to their class definitions and sometimes don't? For example, here reject_criteria gets linked to the py3 standard types definition of dict (which incidentally, to me doesn't seem all that helpful); then in the next codeblock the variable name epochs does not get linked to the class constructor, but in the code block after that, aud_epochs does get linked to the class constructor.

  2. Why aren't methods linked? Here the variable power gets linked to the AverageTFR object, but the method part of power.plot(...) does not get linked to the docs for its plot method

To me it seems like if methods don't get links within code blocks, and if variable names don't consistently get links within code blocks, maybe the only thing that should get links within code blocks is functions / class constructors (i.e., the things that are now bolded). WDYT?

@larsoner
Copy link
Member Author
larsoner commented Jan 6, 2020

But I have two questions:

These are deficiencies in the SG implementation that should be fixed at some point. For example sphinx-gallery/sphinx-gallery#584 might help with one or both of those. Others will require fixing other things within the get_mapping function probably.

To me it seems like if methods don't get links within code blocks, and if variable names don't consistently get links within code blocks, maybe the only thing that should get links within code blocks is functions / class constructors (i.e., the things that are now bolded). WDYT?

I'd rather fix the SG drawbacks if possible (probably in a separate PR to SG)

agramfort
agramfort approved these changes Jan 6, 2020
View reviewed changes
Copy link
Member
@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@drammock merge if happy

@drammock
Copy link
Member
drammock commented Jan 6, 2020

I'd rather fix the SG drawbacks if possible (probably in a separate PR to SG)

OK, fair enough. To me it still seems unnecessary / overkillI to, e.g., link every variable name that is a dict to the standard library definition of dict. If it were possible in SG to selectively turn some kinds of auto links on/off, I would certainly consider turning off the linking of variable names to their type definitions, and only linkify the class constructors / functions / methods. But you're right that this should be solved in SG, not here.

@drammock drammock merged commit c437ed0 into mne-tools:master Jan 6, 2020
@larsoner
Copy link
Member Author
larsoner commented Jan 6, 2020

Based on what's in SG now you can change via CSS the sphx-glr-backref-instance links however you want. So for example you can make them the same color as the uninteresting text. That way the links are there if people want them but they don't stand out as much

@larsoner larsoner deleted the doc branch January 6, 2020 21:25
@agramfort
Copy link
Member
agramfort commented Jan 7, 2020 via email

@lucyleeow lucyleeow mentioned this pull request Jan 7, 2020
Merged
@larsoner larsoner mentioned this pull request Jan 7, 2020
Merged
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@larsoner @AdoNunes
MRG, DOC: Improve styling using classes (mne-tools#7174)
f1047ad 
* DOC: Improve styling using classes [skip travis]

* CI: Touch example [travis skip]

* FIX: Revert branch [skip travis]
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@larsoner @AdoNunes
MRG, DOC: Improve styling using classes (mne-tools#7174)
1fb6807 
* DOC: Improve styling using classes [skip travis]

* CI: Touch example [travis skip]

* FIX: Revert branch [skip travis]
@cbrnr cbrnr mentioned this pull request Jun 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@larsoner @drammock @agramfort
0