search

magma1447 / greasemonkey-geocaching-projectgc

Adds links and data to Geocaching.com to make it collaborate with Project-GC.com
MIT License
18 stars 14 forks source link

Fixed #113 #129

Closed rteitelman closed 1 year ago

rteitelman commented 1 year ago

Fixed issue #113 and made a few minor wording changes to the settings menu.

magma1447 commented 1 year ago

To me this is good enough to accept, but I let @jewettg do the work since he is the one that's most active with the repository right now. I do have a few suggestions for the future though.

  1. Make sure the indentation (space/tabs) are aligned with the rest of the code. This might require adjusting the editor to match the existing code. As you can see in the above link your code is further to the right than the rest. It shouldn't be. I think however that this can be fixed while approving the push request, but I am not 100% certain. My GitHub skills are a bit limited.
  2. In general, no need to leave the old code. That's where Git comes in, all code is version controlled and one can use Git to see the history of the code, and compare any version with any other version. With that said, I often do similar things myself, because it's easier for me on things I work on alone.
  3. Try to only submit one kind of patch per commit/push request. In this specific commit you have code for two patches. First off you are renaming most settings, secondly you are fixing the metric/imperial handling. Best practices is to make one commit per such change. Not the biggest deal in this case though.

Regardless - I understand that this isn't your usual playground and that you really are trying to help out. So don't see it as meaningless criticism. I am just trying to make it easier for all of us in the future.

rteitelman commented 1 year ago

Ok, we'll let jewettg make the final review.

  1. I understand your comments, for future assignments.
  2. I told JewettG that the issue really needing his expertise is #108 (Setting Menu reloading pages). It's a mystery for sure.
  3. Didn't you say that there would be a fix soon on the project-gc side for the number of "your" found logs? Issue #115
  4. As far as I can tell, all other issues can be closed. They really never were issues (just my inquiries) or fixed.
magma1447 commented 1 year ago
  1. Didn't you say that there would be a fix soon on the project-gc side for the number of "your" found logs? Issue Incorrect "Yours" count on the log summary line showing count of: All Logs, Yours, Friends, Gallery #115

Soon is not yet, it will come with the next release of Project-GC. It may very well take another week. Depends on other things in pipeline.

jewettg commented 1 year ago

@rteitelman - Please remove the old code (per @magma1447), but leave the comments in about the fixes, and issues fixed. I think that is helpful. Be brief, do not repeat language in the issue, just put: # Issue xxx; fixed yyyy-mm-dd (very brief description)

rteitelman commented 1 year ago

OK, I have updated my file greasemonkey-geocaching-projectgc.user.js per the suggestion. Do you see it now?