-
Notifications
You must be signed in to change notification settings - Fork 74
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
DOC: readme tweaks #2113
DOC: readme tweaks #2113
Conversation
c797ab5
to
aa1d2cb
Compare
I don't think PyPA likes your README. 😬 |
As for the content, seems weird to mention Specviz but then point to Cubeviz notebook. And should add mention of Specviz2D while you are at it? |
Just finding out the |
0df5b7e
to
a4d8da8
Compare
Experimenting some more. The centering issue is blocked by github/markup#163 – I tried a few work arounds, none worked. |
a4d8da8
to
6279084
Compare
I have resolved to ditch the centering effort. This PR now has my tiniest jdaviz diff ever! |
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.
suuuuper controversial now, but I think we can live with it 😉
While you are at it, might as well point the notebook mention to be consistent with the import mention, and add Specviz2D? |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #2113 +/- ##
==========================================
+ Coverage 91.90% 91.94% +0.03%
==========================================
Files 143 143
Lines 15605 15674 +69
==========================================
+ Hits 14342 14411 +69
Misses 1263 1263 see 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
I'm not sure I follow the 2D part. If we change the links from Cubeviz -> Specviz, then we're only mentioning Specviz. Should we additionally mention Specviz2D, or all of the other helpers too? |
I took the liberty to add Specviz2D in a second commit. If you don't like it, feel free to drop it. Thanks! |
Co-authored-by: P. L. Lim <[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!
The README suggests displaying the app with
specviz.app
, when we usually recommendspecviz.show()
, so I updated it.Also, @orifox once requested that we center-align the logo. I tried to sort that out, but it looks like github still doesn't fully support the necessary rst features.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.