-
Notifications
You must be signed in to change notification settings - Fork 212
feat: add fixed position coordinate picker with map interface #909
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
base: main
Are you sure you want to change the base?
Conversation
|
@dzienisz is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
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.
Overall really glad to see this has been added to the code base, it has been needed for a while.
I will need to see some changes reverted otherwise it will cause all kinds of type issues around the code base. Additionally, the Fixed Position component must be added to the DynamicFormField and referenced using the type field by its string value (known as a factory function). Once you look at the DynamicFormField component you'll see the pattern. I'm available to help answer any questions on will be required to get this merged
| device.log.trace( | ||
| Types.Emitter[Types.Emitter.HandleFromRadio], | ||
| `🚧 Received Queue Status: ${decodedMessage.payloadVariant.value}`, | ||
| `🚧 Received Queue Status: ${JSON.stringify(decodedMessage.payloadVariant.value)}`, |
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.
Unsure why this was added, it should be reverted as this would touch every piece of data received by the web app from a node.
| }, | ||
| "altitude": { | ||
| "label": "Altitude", | ||
| "description": "Meters above sea level (optional)" |
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 should accept an interpolated string, as I believe if someone's localisation is set to US, it will be feet above sea level. It would look like {{ unitOfMeasure }} in other translations you can see examples of this usage.
So the string should look like {{ unit }} above sea level (optional) and pass in the Localization protobuf value.
| disabledBy?: DisabledBy<T>[]; | ||
| label: string; | ||
| description?: string; | ||
| description?: React.ReactNode; |
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 should be reverted. I think you modified the base field instead of adding your specific fields to the DynamicFormField.tsx file, and calling it from a string in the config.
| label: string; | ||
| fieldName: string; | ||
| description?: string; | ||
| description?: React.ReactNode; |
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.
Revert same reason as above.
| currentPosition?.altitude ? String(currentPosition.altitude) : "" | ||
| ); | ||
|
|
||
| const handleMapClick = useCallback((event: any) => { |
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.
Event should be typed correctly. Any cannot be used. This should be something like React.ChangeEvent<HTMLDivElement>>
| <> | ||
| {t("position.fixedPosition.description")} | ||
| {formValues.fixedPosition && ( | ||
| <FixedPositionPicker |
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 need to be used in the same way as other Form fields in the code base. You'll need to add this as a field to DynamicFormField.tsx and call it using the correct string. There should be lots of examples of this in our code base to show you how its all connected.
|
I am open to all suggestions. This is my first big PR. I can improve it next week. |
f4ffb1b to
d3777e6
Compare
- Added new FixedPositionPicker component with interactive map and coordinate inputs - Enhanced position settings UI to allow manual coordinate entry and map-based selection - Added success/error toast notifications for position updates - Updated translations to include new fixed position coordinate picker labels and messages - Added support for requesting current position updates from device - Fixed queue status logging to properly stringify
d3777e6 to
9b80d5e
Compare
|
@dzienisz Wanted to follow up and see if the fixes were going to be added to this PR? |
today, sorry for delay |
Description
Related Issues
Fixes: Users cannot set fixed position coordinates on web client
Changes Made
New Components:
Modified Files:
initialViewStateprop to support custom zoom levels and center coordinatesJSON.stringify()instead of string concatenationKey Features:
setFixedPositionadmin message with coordinatesgetOwnerRequestTesting Done
Screenshots (if applicable)
[Add screenshots showing the interactive map picker, coordinate inputs, and buttons]
Checklist
Additional Notes: