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

Fix/documentation supplement in address search #221

Closed

Conversation

jedi-of-the-sea
Copy link
Contributor

@jedi-of-the-sea jedi-of-the-sea commented Jan 28, 2025

Summary

The configuration examples in AddressSearch were not sufficient in case of the bkg and mpapi search methods. To prevent a linting error, it is necessary to typecast the queryParameters to BKGParameters or MpApiParameters.

Instructions for local reproduction and review

Pull Request Checklist (for Reviewee)

  • Changelogs are maintained - not relevant
  • Functionality has been tested in Firefox, Chrome, Safari - not relevant
  • Functionality has been tested on a smartphone - not relevant
  • Functionality has been tested with 200% screen zoom - not relevant
  • Screenreader functionality has been manually tested with NVDA - not relevant

UI has been tested in the following tools regarding accessibility (only regarding functionality affected in this PR) - not relevant

  • Chrome Lighthouse - not relevant
  • Firefox Accessibility - not relevant

@jedi-of-the-sea jedi-of-the-sea self-assigned this Jan 28, 2025
@jedi-of-the-sea jedi-of-the-sea added the documentation Improvements or additions to documentation label Jan 29, 2025
Copy link
Member

@warm-coolguy warm-coolguy left a comment

Choose a reason for hiding this comment

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

I would like to reject this pull request since it changes documentation facing users that currently do not have access to our type files (they're not part of the published packages), and we know most of them use .js anyway. Furthermore, examples explicitly refer to javascript (as seen on code fences: "```js") and this wouldn't be valid syntax there.

@jedi-of-the-sea Should you agree, please close this pull request.

@jedi-of-the-sea
Copy link
Contributor Author

jedi-of-the-sea commented Jan 30, 2025

I would like to reject this pull request since it changes documentation facing users that currently do not have access to our type files (they're not part of the published packages), and we know most of them use .js anyway. Furthermore, examples explicitly refer to javascript (as seen on code fences: "```js") and this wouldn't be valid syntax there.

@jedi-of-the-sea Should you agree, please close this pull request.

@warm-coolguy I partly wanted to document this for myself because in a few month I might have forgotten about this problem and will be confused again 😅 But I agree that the example configurations should not be altered if they are for javascript. Maybe a hint like "Please note that you need to specify the type queryParameters as BKGParameters / MpApiParameters when using typescript" after the corresponding documentation of type=bkg / type=mpapi? Would that be a good compromise?

@warm-coolguy
Copy link
Member

warm-coolguy commented Jan 30, 2025

@jedi-of-the-sea Since our users can't use our typescript types, that's still somewhat confusing to them; I'd expect to have a usable .d.ts file on reading that, and there'd be none to find.

Alternative suggestion: We extend the QueryParameters interface to

/**
 * Additional queryParameters for the GET-Request;
 * for the specific parameters for each request,
 * please refer to the types in the plugin
 */
export interface QueryParameters {
  /** sets the maximum number of features to retrieve */
  maxFeatures?: number
  /** depending on backend service, anything may belong to the query */
  [key: string]: unknown
}

That's a little unspecific, but saves us various typecasts, and is ultimately true.

@dopenguin Requesting approval for change.

@dopenguin
Copy link
Member

I'd go along with

I would like to reject this pull request since it changes documentation facing users that currently do not have access to our type files (they're not part of the published packages), and we know most of them use .js anyway. Furthermore, examples explicitly refer to javascript (as seen on code fences: "```js") and this wouldn't be valid syntax there.

@jedi-of-the-sea Should you agree, please close this pull request.

However, it may become interesting again if we deliver d.ts-files along with out package, as mentioned in #221 (comment). But that would probably need a merge of e.g. #108 and an upgrade to vue@3 - so nothing relevant for now.

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

Successfully merging this pull request may close these issues.

3 participants