Skip to content
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

Merged
merged 4 commits into from
Mar 24, 2023
Merged

DOC: readme tweaks #2113

merged 4 commits into from
Mar 24, 2023

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Mar 24, 2023

The README suggests displaying the app with specviz.app, when we usually recommend specviz.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

  • Is a change log needed? If yes, is it added to 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, maintainer
    should 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.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@bmorris3 bmorris3 added documentation Explanation of code and concepts trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Mar 24, 2023
@bmorris3 bmorris3 added this to the 3.5 milestone Mar 24, 2023
@bmorris3
Copy link
Contributor Author

bmorris3 commented Mar 24, 2023

Preview:

Screen Shot 2023-03-24 at 10 26 00

We could also center the badges, any preference?

@pllim
Copy link
Contributor

pllim commented Mar 24, 2023

I don't think PyPA likes your README. 😬

@pllim
Copy link
Contributor

pllim commented Mar 24, 2023

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?

@bmorris3
Copy link
Contributor Author

Just finding out the raw directive is forbidden on PyPI... investigating.

README.rst Outdated Show resolved Hide resolved
@bmorris3 bmorris3 force-pushed the readme-improve branch 6 times, most recently from 0df5b7e to a4d8da8 Compare March 24, 2023 15:04
@bmorris3
Copy link
Contributor Author

Experimenting some more. The centering issue is blocked by github/markup#163 – I tried a few work arounds, none worked.

@bmorris3
Copy link
Contributor Author

I have resolved to ditch the centering effort. This PR now has my tiniest jdaviz diff ever!

Copy link
Member

@kecnry kecnry left a 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 😉

@pllim
Copy link
Contributor

pllim commented Mar 24, 2023

While you are at it, might as well point the notebook mention to be consistent with the import mention, and add Specviz2D?

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03 🎉

Comparison is base (5510486) 91.90% compared to head (cbc0db2) 91.94%.

❗ Current head cbc0db2 differs from pull request most recent head a5910a2. Consider uploading reports for the commit a5910a2 to get more accurate results

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bmorris3
Copy link
Contributor Author

add Specviz2D?

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?

@pllim
Copy link
Contributor

pllim commented Mar 24, 2023

I took the liberty to add Specviz2D in a second commit. If you don't like it, feel free to drop it. Thanks!

README.rst Outdated Show resolved Hide resolved
Co-authored-by: P. L. Lim <[email protected]>
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks!

@pllim pllim merged commit 8a8af3f into spacetelescope:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Explanation of code and concepts no-changelog-entry-needed changelog bot directive trivial Only needs one approval instead of two
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants