PeWu / topola

Topola – online genealogy visualization
Apache License 2.0
92 stars 27 forks source link

Added originalSizeUrl to JsonImage #45

Closed czifumasa closed 2 years ago

czifumasa commented 2 years ago

Hi, I'd like to add a new optional field to JsonImage in topola API. I am going to use it in my next PR to topola-viewer for displaying photos in details panel.

Currently when data is fetched from wikitree api, small thumbnails are used to display photos on charts. I would like to keep it like that, cuz thumbnails are perfectly fine for chart. But in details panel we have more space, and 75px thumbnail looks reaally bad. I noticed that with wikitree api I can easily grab url for original full-size photo, and save it in JsonImage so it can be used later to build gedcom file and display original photo in details panel.

PeWu commented 2 years ago

You shouldn't need to make this change here. The details panel in topola-viewer reads information from the GEDCOM data (GedcomData), not from the object passed to the topola library which only handles rendering of the chart.

You should be able to achieve this modifying only https://github.com/PeWu/topola-viewer/blob/master/src/datasource/wikitree.ts

Something like:

interface PersonData {
  indi: JsonIndi;
  imageUrl?: string;
}

function convertPerson(person: Person, intl: IntlShape): PersonData {
  // ...
  const imageUrl = /* extract URL from Person */
  return {indi, imageUrl};
}

function indiToGedcom(indi: PersonData): GedcomEntry {
  // Add image URL to GecomEntry
}

// and passing around PersonData instead of JsonIndi
czifumasa commented 2 years ago

I wanted to do exactly what you described, see my commit on fork, but buildGedcom method in wikitree.ts is using chartData with JsonGedcomData, typing from topola, so I assumed it would be much easier to just extend topola API.

I could probably find some other solution(for example: pass map to buildGedcom method with thumbnail image as key and original image as value and use it later for replacing URLs in Gedcom for details panel). If you think that workound is better aproach or have some other idea, I'll adjust my implementation!

PeWu commented 2 years ago

There is no need in extending the topola API with something that is not used by the topola library.

Maybe the easiest would be to pass to buildGedcom a map from person id to image URL?

An alternative would be to use a bit of TypeScript typing magic and do something like this:

interface AdditionalPersonData {
    imageUrl?: string;
}
type PersonData = JsonIndi & AdditionalPersonData;

and using PersonData instead of JsonIndi, sometimes needing to cast indi as PersonData. However, I find this approach a bit less readable.

czifumasa commented 2 years ago

I followed your suggestions with passing map from person id to image URL and it works perfectly fine 👍

See my topola-viewer PR in : https://github.com/PeWu/topola-viewer/pull/100

So API change is no longer necessary and I will close this PR