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

(solves 736) Checkbox to toggle latitude and longitude in Venue block #877

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

stephenerdelyi
Copy link
Collaborator

@stephenerdelyi stephenerdelyi commented Sep 17, 2024

Description of the Change

Closes #736

How to test the Change

Changelog Entry

Added - New "Latitude/Longitude" select dropdown that allows a user to enter or edit custom lat/lng values

Credits

Props @stephenerdelyi , @jmarx

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@carstingaxion
Copy link
Collaborator

I tested your work the first time, and it showed the UI nicely.

Unfortunately, the current set of demo-data has no lat lng data. Wasn’t there an idea in the past to request missing data on the fly to help with backwards compatibility?

Next to that, I would prefer to have this fields in the Venue Settings, rather than in the map settings, as the data belongs to the venue, not the kind of map rendering.

@mauteri mauteri changed the base branch from main to develop October 10, 2024 13:00
</PanelRow>
{mapCustomLatLng && (
<>
<TextControl
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a NumberControl

import { __experimentalNumberControl as NumberControl } from '@wordpress/components';

updateVenueMeta({ latitude: value });
}}
/>
<TextControl
Copy link
Contributor

Choose a reason for hiding this comment

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

NumberControl here too. If you leave this as TextControl, hitting a non-number crashes the block.

Broadcaster({ setLongitude: value });
updateVenueMeta({ longitude: value });
}}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a button that will "Reset" the coordinates. Just call the button "Reset" and have it take the address and run it through the API again to bring back original values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that the changes in lat/long are not being retained. I did some updates, but I don't think anything that should have effected functionality. It changes, but then when I leave focus, it reverts... It might be good to use the redux stores rather than Broadcaster/Listener as that may be the issue.

Copy link
Collaborator

@jmarx jmarx Nov 4, 2024

Choose a reason for hiding this comment

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

We should have a button that will "Reset" the coordinates. Just call the button "Reset" and have it take the address and run it through the API again to bring back original values.

toggling default/custom will now reset the map in way you are describing.
@mauteri

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems that the changes in lat/long are not being retained. I did some updates, but I don't think anything that should have effected functionality. It changes, but then when I leave focus, it reverts... It might be good to use the redux stores rather than Broadcaster/Listener as that may be the issue.

We implemented the store. So all these issues should be resolved. @mauteri

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmarx I would like to add the Reset button. I get what you are saying, but that might not be super obvious to people. A reset button would be.

@@ -23,7 +25,7 @@ import { isVenuePostType } from '../../helpers/venue';
import VenueSelector from '../../components/VenueSelector';
import VenueInformation from '../../panels/venue-settings/venue-information';
import OnlineEventLink from '../../components/OnlineEventLink';
import { Listener } from '../../helpers/broadcasting';
import { Broadcaster, Listener } from '../../helpers/broadcasting';
Copy link
Contributor

Choose a reason for hiding this comment

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

If you'd like to look at the store I created for updates to the Event Date block, you can start using that instead of the Listener / Broadcaster. If not, we can update to that later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you'd like to look at the store I created for updates to the Event Date block, you can start using that instead of the Listener / Broadcaster. If not, we can update to that later.

@mauteri Done.

Broadcaster({ setLongitude: value });
updateVenueMeta({ longitude: value });
}}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that the changes in lat/long are not being retained. I did some updates, but I don't think anything that should have effected functionality. It changes, but then when I leave focus, it reverts... It might be good to use the redux stores rather than Broadcaster/Listener as that may be the issue.

@jmarx
Copy link
Collaborator

jmarx commented Nov 4, 2024

@mauteri We are pretty close on this. Everything works well with OSM. But we just need to make another adjustment for Google Maps. But feel free to take another pass at this and check out our implementation of the redux store. Please let me know if anything needs to be changes with that.

Also,
I personally don't think we need the reset button. If you test this pr, you'll find that the lat/long toggle(use default/use custom) will take care of all that.

@stephenerdelyi stephenerdelyi changed the title DRAFT: (solves 736) Checkbox to toggle latitude and longitude in Venue block (solves 736) Checkbox to toggle latitude and longitude in Venue block Nov 9, 2024
Copy link

github-actions bot commented Nov 9, 2024

Preview changes with Playground

You can preview the recent changes for PR#877 with the following PHP versions:

PHP Version 8.3

PHP Version 7.4

Download .zip with build changes

Made with 💙 from GatherPress & a little bit of WordPress Playground. Changes will not persist between sessions.

@stephenerdelyi
Copy link
Collaborator Author

@mauteri @jmarx this is ready for final review. Got the typo / JS formatting / tests updated and cleaned things up.

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

Successfully merging this pull request may close these issues.

Checkbox to toggle latitude and longitude in Venue block
4 participants