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

feat(viewer): resolve geoname id and show name (DSP-1212) #305

Merged
merged 6 commits into from Jun 16, 2021

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Jun 15, 2021

resolves DSP-1212

This PR adds a service that resolves a geoname id.

@gautschr @kilchenmann I'd like to discuss whether and to what degree we could also implement the autocomplete functionality for creating / editing values (like in DSP-Tangoh). As for the backend, we decided not to reimplement the logic with hierarchical lists as it was implemented in SALSAH.

@tobiasschweizer
Copy link
Contributor Author

As for the token (which is actually more of a username), I made a new entry in the config object. Any user of the app could see it in the network requests to geonames so I thought it does not make much sense to hide it ...

@gautschr
Copy link
Contributor

Your suggested autocomplete functionality for creating / editing values would be very comfortable in cases where the data are modified or created. It would eliminate an intermediate time-consuming step where one has to go to geonames to search for the correct ID.

Copy link
Contributor

@gautschr gautschr left a comment

Choose a reason for hiding this comment

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

Works perfectly!

@tobiasschweizer
Copy link
Contributor Author

Your suggested autocomplete functionality for creating / editing values would be very comfortable in cases where the data are modified or created. It would eliminate an intermediate time-consuming step where one has to go to geonames to search for the correct ID.

Ok, I can implement that functionality in a subsequent PR.

Copy link
Contributor

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

Should the name of the corresponding Geoname identifier be added to the delete confirmation popup?

Screen Shot 2021-06-16 at 15 23 52

@tobiasschweizer
Copy link
Contributor Author

tobiasschweizer commented Jun 16, 2021

Should the name of the corresponding Geoname identifier be added to the delete confirmation popup?

Screen Shot 2021-06-16 at 15 23 52

Good question. Ideally, yes because the name would make it clear to the user what is going to be deleted.

In practice, there are two problems:

  • the name is not contained in the ReadValue itself, but the ConfirmationDialogComponent deals with a ReadValue
  • the name is obtained asynchronously via the geoname API and displayed using the async pipe. So we don't really know when it's available and delegate this to Angular.

EDIT: the case of a color value is actually similar. The user sees the hex code but not the color ;-)

@mdelez
Copy link
Contributor

mdelez commented Jun 16, 2021

Should the name of the corresponding Geoname identifier be added to the delete confirmation popup?
Screen Shot 2021-06-16 at 15 23 52

Good question. Ideally, yes because the name would make it clear to the user what is going to be deleted.

In practice, there are two problems:

  • the name is not contained in the ReadValue itself, but the ConfirmationDialogComponent deals with a ReadValue
  • the name is obtained asynchronously via the geoname API and displayed using the async pipe. So we don't really know when it's available and delegate this to Angular.

EDIT: the case of a color value is actually similar. The user sees the hex code but not the color ;-)

All good, no need to spend time on it in my mind

@mdelez mdelez self-requested a review June 16, 2021 14:18
@tobiasschweizer tobiasschweizer merged commit 18ea19b into main Jun 16, 2021
@tobiasschweizer tobiasschweizer deleted the wip/dsp-1212-geoname branch June 16, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants