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

Venue handling refactor #382

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Conversation

ammapspeople
Copy link
Contributor

What

  • Rewrote venue handling in the Map Template.

Why

  • Several bugs related to venues made us do a more thorough refactor.

How

  • A new custom hook, useCurrentVenue that isolates side effects to changing venue
  • Refactor the naming of the timing controls in the useMapBoundsDeterminer hook to be more clear about what they are.
  • Removed the venueState atom since it is not necessary, and having several separate states for the venue is confusing.
  • Cleaned up unnecessary calls to set venue.
  • Introduced a venueWasSelected boolean atom to indicate if the user has selected a venue. This is used to be able to switch the venue on the map.
  • Only set the "AUSTINOFFICE" as venue if there is no apiKey and no venue set.

…o the useCurrentVenue hook where it belongs.
…entVenue hook, since it is a side effect of venue changes.
…nfusing to have a state for both the venue name and the actual venue.
Comment on lines 123 to 125
* When map position is known, run callback.
*/
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the description here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see to it.

Comment on lines 131 to 135
/*
* When venue is changed on the map, run callback.
* Set the current venue name whenever the venue is changed on the map.
*/
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should update the description here as well based on the new logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🫡

@@ -74,23 +69,18 @@ const useMapBoundsDeterminer = () => {
*/
useEffect(() => {
determineMapBounds();
}, [mapsIndoorsInstance, venue, venues, locationId, kioskOriginLocationId, pitch, bearing, startZoomLevel, categories]);
}, [mapsIndoorsInstance, currentVenueName, locationId, kioskOriginLocationId, pitch, bearing, startZoomLevel, categories]);

/**
* Based on the combination of the states for venueName, locationId & kioskOriginLocationId,
* determine where to make the map go to.
*
* @param {string} [forcedVenue] - If set, this venue will be used instead of the current venue.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this param should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👀 🫡

Copy link
Contributor

@andreeaceachir142 andreeaceachir142 left a comment

Choose a reason for hiding this comment

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

✅ 🆗 🆗 🆗

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