MAAP-Project / stac_ipyleaflet

Apache License 2.0
29 stars 2 forks source link

Clean up core, make widget and tab creation dynamic, add utilities dir #86

Closed sandrahoang686 closed 1 year ago

sandrahoang686 commented 1 year ago

Summary:

The goal of this PR was not to extensively test, that would be too much scope for this ticket... The goal is to add initial tests and to start breaking out code better and create for patterns that would help with testability

Fixes or Addresses Issue #: [ticket number & link]

https://github.com/MAAP-Project/stac_ipyleaflet/issues/88

Checklist before requesting a review:

sandrahoang686 commented 1 year ago

Great changes! Although I do think this PR could have been split up into separate chores/ and test/ changes.

@emmalu thank you!! I see what you are saying! That is definitely one way this could have been approached but I actually prefer this be one PR (really this is all preference based in the end) BUT I do think its important to keep track of the types of changes made so I tried to do meaningful commits as I went to tackle specific parts. PRs hold a history of a group of commits that were merged and in the end the main branch would be a collection of all these commits. So history wise, it doesn't make too much of a difference in git history land. Smaller / Incremental PRs are great for shipping incrementally and can be better for those doing reviews. I also prefer smaller & incremental PRs but I thought this one is fine as its not to big and we can go based off of commit 🙌🏼