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

Display alternative place names #566

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

Conversation

cajturner
Copy link

This is my first contribution and an attempt to solve issue: #27

I have decided to only tackle displaying alternative place names and not implementing edit functionality. This will be added in a future PR.

Any feedback or suggestions welcome

Screenshots

Place view when alternative name is present
Screenshot 2025-01-05 at 20 53 26
Alternative name Tab when alternative name is present
Screenshot 2025-01-05 at 20 53 28
Alternative name Tab when alternative name is present and in edit mode
Screenshot 2025-01-05 at 20 53 31
Place view when alternative name is not present
Screenshot 2025-01-05 at 20 53 37
Place view when alternative name is not present and in edit mode
Screenshot 2025-01-05 at 20 53 40

@@ -59,6 +60,11 @@ const _allTabs = {
('placeref_list' in data && data?.backlinks?.place?.length >= 0),
conditionEdit: data => 'placeref_list' in data,
},
placeNames: {
title: 'Alternate Names',
condition: data => data.alt_names?.length > 0,
Copy link
Member

Choose a reason for hiding this comment

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

Probably it doesn't matter in practice, but since this function is also called for objects that are not places and thus don't have an alt_names property, I find it clearer to add the ?. also in front of alt_names

@DavidMStraub
Copy link
Member

DavidMStraub commented Jan 6, 2025

Hi,

thanks for taking this on!

Can you add the language as well?

The main issue I see is with date formatting - unfortunately we don't have a proper way in Javascript to represent all of the complexity of Gramps' date formatting, and just using Y-M-D can be misleading (already shown in your example with zeros). This is particularly important as place name dates are likely to always be spans.

I'm afraid the only proper solution would be to add the formatted date to the place profile in the backend first, here: https://github.com/gramps-project/gramps-web-api/blob/master/gramps_webapi/api/resources/util.py#L362

A more radical alternate solution would be to add a new date formatting endpoint in the API where the frontend can post a date object and it returns a string...

@cajturner
Copy link
Author

cajturner commented Jan 6, 2025

Can you add the language as well?

I added the 'Alternate Names' string to strings.js and can see the value translated correctly. Is anything else required?
Screenshot 2025-01-06 at 20 17 55

The main issue I see is with date formatting - unfortunately we don't have a proper way in Javascript to represent all of the complexity of Gramps' date formatting, and just using Y-M-D can be misleading (already shown in your example with zeros). This is particularly important as place name dates are likely to always be spans.

I'm afraid the only proper solution would be to add the formatted date to the place profile in the backend first, here: https://github.com/gramps-project/gramps-web-api/blob/master/gramps_webapi/api/resources/util.py#L362

A more radical alternate solution would be to add a new date formatting endpoint in the API where the frontend can post a date object and it returns a string...

I'm happy to add the formatted date to the place profile backend. I can see locale.date_displayer.display() being used so that looks like it can be reused

@DavidMStraub
Copy link
Member

I'm happy to add the formatted date to the place profile backend. I can see locale.date_displayer.display() being used so that looks like it can be reused

Perfect, let me know if you need guidance.

@cajturner
Copy link
Author

Thanks @DavidMStraub I've been busy since last week but hope to still make the changes to the api. I was struggling a little to get the tests to run on my local machine (mac). I'll give it another go over the weekend

@DavidMStraub
Copy link
Member

The backend development docs haven't been updated in a while, if anything doesn't work as expected when setting up the dev environment, feel free to open an issue about it.

@DavidMStraub
Copy link
Member

Converted to draft as we first need to finish gramps-project/gramps-web-api#608.

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