-
Notifications
You must be signed in to change notification settings - Fork 26
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
Publish web SDK #189
Publish web SDK #189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good; I just have a few nitpicks about documentation and found a few omissions going through it.
I'll push my changes and merge though so we can get this out ASAP.
guide/src/web-getting-started.md
Outdated
|
||
Before getting routes, you’ll need the user’s current location. | ||
You can get this from the location provider. | ||
`BrowserLocationProvider` is a location provider that uses the browser's geolocation API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably cover simulation, but that doesn't need to go in this tutorial.
guide/src/web-getting-started.md
Outdated
Here's an example: | ||
|
||
```javascript | ||
const config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config
object is required for core.startNavigation
, but I'm not sure how to document it correctly. Also, should we include this as a default value for core.startNavigation
in our web component code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config object is required for core.startNavigation
Actually we made a change very recently which moves the config requirement to the FerrostarCore
constructor on other platforms (Android example). I think doing the same on the web would be reasonable too. Most of the time you want to just set a config once. This makes it an optional override when calling startNavigation
.
Also, should we include this as a default value for core.startNavigation in our web component code?
Yeah, the above addresses this, but it should be an optional override with a default set on the FerrostarCore
component. Somewhat related: the startNavigationFromSearch
internal function currently just uses hard-coded values; storing the config at the component level can fix this.
Re: documentation, I think it's good enough for now as the most common path is clear. We can improve customization docs as we refine the design.
Finally, here it is.
Note that the guide is still very preliminary and needs polishing!
Tested and working, although CI and unit tests are missing. Automated testing still needs more work because WebAssembly and web components is not compatible enough with Vitest/Jest, and the introduction of Vitest browser mode introduces more complexity so I want to avoid using browser mode as much as possible.
Closes #193