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

Remove the trailing comma in the timeline event box if no place name #28

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TJBFTV
Copy link

@TJBFTV TJBFTV commented Jan 28, 2025

The event box in the timeline shows a trailing comma after the event date, even when there is no event place.

comma before

In the case where an event has no place, self.ftv.get_full_place_name_from_event(event) returns "" instead of None. Of the various ways to fix this, this PR leaves the test for None intact and adds a test for "" to the else block, to allow for potential future changes to self.ftv.get_full_place_name_from_event().


I'm submitting this trivial cosmetic change as a way to learn the ins and outs of PR submission at Github. Feedback on my submission process is appreciated.

Also, I have five other PRs (so far) to submit for bug fixes and an enhancement on the assumed TODO list. All of them are for family_tree_view_timeline.py. Each PR is about a half-dozen lines or less of contiguous code. Would you like me to submit them all at once, or stagger them over time?

@ztlxltl
Copy link
Owner

ztlxltl commented Feb 2, 2025

Thanks for your PR. I'll try to review this soon.


Since you specifically asked for feedback:

  • A more self-explanatory branch name would be better. (You don't need to try to change that for this PR.)
  • You can explicitly request a review when you are done (this is not a draft, so there do not seem to be any major changes pending). This is especially useful if I request changes and I'm not sure if you're done incorporating the feedback.

I think everything else is fine. Keep in mind that my feedback is just based on what I'm used to. If you contribute to other repos on GitHub, the nuances of the process may be different.


You can submit all your improvements in separate PRs (with separate branches of your fork). If they deal with the timeline, I'll probably look at them soon as well.

@TJBFTV
Copy link
Author

TJBFTV commented Feb 3, 2025

Yes, I am TJB at the Gramps forum, and (of course) TJBFTV for your repository here.

Thanks for the feedback. I did forget to change to my branch at GitHub before I submitted my first PR. I now have a local mentor for git and GitHub-related things, so I expect more generally-expected conventions going forward.

I'll try to remember to indicate when a PR submission is complete, not a draft, so you can take a look at it.

The FamilyTreeView is a wonderful enhancement to Gramps. I don't doubt that it has taken you a couple of years to get to this stage. Compliments to you, sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants