ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Improvements to pg web widgets #968

Closed ejeschke closed 2 years ago

ejeschke commented 3 years ago

Co-authored-by: Eric Jeschke eric@naoj.org

ejeschke commented 3 years ago

@pllim, looking to merge some improvements by our Akamai intern @kyraikeda who worked on improving the pg widgets functionality. I know you guys don't use that, but hoping you can give us a code review to show her how this works for github projects!

pllim commented 3 years ago

Wow, +1,683 −345! I might be able to do a general review but unable to dive deep into pg land. Is that sufficient? Otherwise, it might be more useful to get a review from one of your colleagues who actually use the pg backend.

Also, is this time sensitive?

ejeschke commented 3 years ago

@pllim, no hurry. General, quick code review is fine. This is to fill in the compatibility of pg general widget wrappers compared to Qt and Gtk wrappers.

ejeschke commented 3 years ago

It is curious why @kyraikeda isn't the PR author. Anyways, this person can still get credit via https://docs.github.com/en/github/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors as you see fit.

@pllim, it is because her commits were a PR to a branch on a clone by @naojsoft. But we will definitely add credit using your suggestion above.

Probably want to add a change log summarizing all the breaking changes and new API in this PR.

Will do. Mostly, this is filling in API that is already in Qt and Gtk backends in the Pg backend. But I do think there may be one or two new additions that ended up also in Qt and Gtk.

ejeschke commented 2 years ago

Merged, finally, @kyraikeda. Thank you!