Add mazemap embed to events and meetings #2383

ollfkaih · 2021-12-29T15:57:55Z

Adds a map to the sidebar of events and meetings if the event or meeting has mazemapPoi set,
and adds fields to the edit pages to be able to search for rooms and set the mazemapPoi.

Solved the jest/test issues by bundling mazemap as a node module instead of in pageRenderer, based on this.

Minor issue:

~~The saved room is not shown in the field, but is shown in the dropdown, because I have not figured out how to fetch the name of the room when EventEditRoute is rendering.~~

^ Fixed by using location saved on server

Oh, and this is also based on Tirils prior work in pr #1680

Screenshot for anyone curious

SmithPeder

Very good work.

A couple of comments. (I guess some of them apply to the previous work done by Tiril, but yeah, you'll get the credits and the blame) 😆

(Major) When we select a MazeMapRoom, it's kinda wired that we also have to fill in the normal Location. I get that sometimes one wants to write something in both, but if the event is in R1, then the mazemapPoi field will be translated to R1, Realfagbygget so writing anything in the Location is wired. This is kind of a business-logic problem, but something that should be addressed. I think the solution is to make the fields co-required, meaning at least one has to be filled in.
(Minor) When selecting a room in a create or edit view the map is not visible. I can see arguments in favor of hiding a map in this view. But, when trying to select kinky rooms it could be nice to see that the correct room is selected. We could add the Åpne kart i ny fane label under, so the creator can open the map to check that the correct room was selected.

app/actions/MeetingActions.js

app/components/Search/mazemapAutocomplete.js

app/components/Form/SelectInput.js

app/components/Search/mazemapAutocomplete.js

app/components/MazemapEmbed/index.js

app/components/Search/mazemapAutocomplete.js

app/components/MazemapEmbed/index.js

package.json

app/components/Search/mazemapAutocomplete.js

ollfkaih · 2022-01-08T13:04:26Z

I share your sentiment on having only one location field at a time. Perhaps a checkbox ("[x] Bruk mazemap") to toggle whether you type in the freetext location, or if you search for a room and then location gets set to that rooms displayname, would be a good solution.

I skipped adding a map to the event view because I wanted to keep the scope down for this PR so that i could finish it, but did originally consider displaying search results on the map. It would be easy, however, to display the room you've selected (not all results). This would make the sidebar even longer, unless the map is displayed below the description, but that would be a weird place to have the map when the search field is in the sidebar... A link seems like a decent solution.

Also, I'm no UX designer, but I think the colors should be more Abakus and less jarring (blue polygon and purple marker). I'll change them to something more red, but some feedback is appreciated.

SmithPeder · 2022-01-08T13:34:42Z

the colors should be more Abakus and less jarring (blue polygon and purple marker). I'll change them to something more red, but some feedback is appreciated.

I think the red looks great 👍🏻 Maybe @ivarnakken has some nit-picking 😆

This would make the sidebar even longer, unless the map is displayed below the description, but that would be a weird place to have the map when the search field is in the sidebar... A link seems like a decent solution.

yeah, I agree that showing the embedded map can be a bit much. I was thinking a link, or a "Open in new window icon" like this:

ollfkaih · 2022-01-08T14:20:50Z

a "Open in new window icon" like this

That is an elegant solution.
Also, there are some oddities with the map's zLevelControl when changing rooms (it is not removed) due to how mapbox handles its container, so it would require some changes to the embed to display changing mazemapPois on the same map (one could of course create a completely new div on each change, but that seems wasteful).

SmithPeder · 2022-01-08T15:47:20Z

I share your sentiment on having only one location field at a time. Perhaps a checkbox ("[x] Bruk mazemap") to toggle whether you type in the freetext location, or if you search for a room and then location gets set to that rooms displayname, would be a good solution.

yeah, something like that. It's still nice to have the location field set, because the location is used in multiple places, like here

ollfkaih · 2022-01-09T14:20:05Z

I've just added a toggle that switches between mazemap/location fields. Someone more familiar with redux forms might be able to simplify this code, but it seems to work correctly.

ollfkaih · 2022-01-09T14:20:50Z

app/routes/meetings/components/MeetingEditor.js

+            {
+              // TODO: Bug; meeting is not updated when the field above is changed, so the link
+              // will go to whatever room mazemapPoi was set to when the editor was opened
+              /*meeting.mazemapPoi > 0 && (
+              <MazemapLink mazemapPoi={meeting.mazemapPoi} linkText="↗️" />
+            )*/


In the editor for events, I've also added a small icon to link externally to mazemap for the given poi, but I couldn't make this work for meetings due to meeting not updating on every change to the field, like event does in the event edtior. I've left this as a comment for now.

The reason for this is that the state of the form (both the event and the meeting, is stored in the form state in redux. This is extracted for each of the editors, so that we can use the current form values in the components again.

If you look in the EventEditRoute, in the mapStateToProps, you can see that the event prop consists of the current event and all the values from the event form overriding any properties. This is the valueSelector that is used.

You can also see in the MeetingEditRoute that the valueSelector is used to get the invitingUsers. So if you want to use the current mazeMapPoi form value, just override that prop on the meeting object with the one from the valueSelector just as you have done with events. Then this should work great 😄

…tings

You can now toggle between location textfield and mazemap

and moved hooks to more appropriate location

Maybe drone can find mazemap now (?)

.drone.yml

SmithPeder

LGTM 👍🏻

PeterJFB

Looks good to me! 😍

LudvigHz

Nice work! This is a really great feature 👍. Just fixup the comment/TODO in the meeting editor and make sure SSR is good then we can merge this.

app/routes/events/components/EventEditor/index.js

LudvigHz · 2022-02-02T21:25:25Z

app/routes/meetings/components/MeetingEditor.js

+            {
+              // TODO: Bug; meeting is not updated when the field above is changed, so the link
+              // will go to whatever room mazemapPoi was set to when the editor was opened
+              /*meeting.mazemapPoi > 0 && (
+              <MazemapLink mazemapPoi={meeting.mazemapPoi} linkText="↗️" />
+            )*/


The reason for this is that the state of the form (both the event and the meeting, is stored in the form state in redux. This is extracted for each of the editors, so that we can use the current form values in the components again.

If you look in the EventEditRoute, in the mapStateToProps, you can see that the event prop consists of the current event and all the values from the event form overriding any properties. This is the valueSelector that is used.

You can also see in the MeetingEditRoute that the valueSelector is used to get the invitingUsers. So if you want to use the current mazeMapPoi form value, just override that prop on the meeting object with the one from the valueSelector just as you have done with events. Then this should work great 😄

app/components/MazemapEmbed/index.js

ollfkaih · 2022-02-08T21:25:48Z

Oh, if that's the case I guess you can merge it.😁

________________________________ From: Ludvig Hozman ***@***.***> Sent: Tuesday, February 8, 2022 10:18:59 PM To: webkom/lego-webapp ***@***.***> Cc: Olav Kihle ***@***.***>; Mention ***@***.***> Subject: Re: [webkom/lego-webapp] Add mazemap embed to events and meetings (PR #2383) @LudvigHz commented on this pull request.

________________________________ In app/components/MazemapEmbed/index.js<#2383 (comment)>:

+ embeddedMazemap.zLevel = poi.properties.zLevel;

+ }; + + const height = embeddedMazemap.getCanvas().clientHeight; + const maxHeight = height - 50; // 50 pixels account for margins and spacing + embeddedMazemap.zLevelControl.setMaxHeight(maxHeight); + }); + }, [mazemapPoi]); + + return ( + <> + <div style={{ height: props.height || 400 }} id="mazemap-embed" /> + <MazemapLink mazemapPoi={mazemapPoi} linkText={props.linkText} /> + </> + ); +}; Soo, I just tested this with the latest from your branch, and both events and meetings seem to work just fine? 🥲 Note that sometimes just after you start up the server, you might need to hard refresh a couple times before it's completely stable (due to caching and some weird states and all that). But anyways, this looks really good now. And afaikt this is good to go! — Reply to this email directly, view it on GitHub<#2383 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/APKFRHL4MBP77PJSDA6O7KLU2GCEHANCNFSM5K6GAVNA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

LudvigHz · 2022-02-08T21:29:15Z

Oh, if that's the case I guess you can merge it.grin

Actually, hold that thought. It seems that for me, joblistings is broken with the Element is not defined error coming from Mazemp 🤔. I don't know why that is, but it seems that the mazemap library is being loaded in some places when it absolutely should not. So I think this needs some more investigating sadly

ollfkaih · 2022-02-08T21:51:48Z

Could it be in or related to SelectInput.js?
Edit: had some faulty uncommited code locally, but reproduced the errors with joblistnings.

LudvigHz · 2022-02-08T22:40:20Z

Could it be in or related to SelectInput.js? Edit: had some faulty uncommited code locally, but reproduced the errors with joblistnings.

See my latest reply in the thread here: #2383 (comment)

LudvigHz · 2022-02-09T12:37:59Z

app/components/Search/mazemapAutocomplete.js

+      const { Mazemap } = this.state;
+      const handleSearch = (query: string): void => {
+        if (!query || !Mazemap) {
+          return;
+        }
+        const mazemapSearchController = new Mazemap.Search.SearchController({
+          campusid: 1,
+          rows: 10,
+          withpois: true,
+          withbuilding: false,
+          withtype: false,
+          withcampus: false,
+          resultsFormat: 'geojson',
+        });
+        this.setState({
+          searching: true,
+        });
+        mazemapSearchController
+          .search(query)
+          .then((results) => {
+            if (this._isMounted) {
+              this.setState({
+                result: results.results.features.map((result) =>
+                  mapRoomAndBuildingToResult(
+                    result.properties.dispPoiNames[0],
+                    result.properties.dispBldNames[0],
+                    result.properties.poiId
+                  )
+                ),
+              });
+            }
+          })
+          .finally(() => {
+            if (this._isMounted) {
+              this.setState({ searching: false });
+            }
+          });
+      };


Hmm, why did you move these methods into render()? This will make every render heavier as it sets up new functions for every render and not when creating the component as before.

Also, it's probably alot better to initialize the Mazemap.Search.SearchController only once in the mount when you do the import, and rather store that in state instead of the entire Mazemap lib, since it seems that that's the only thing used here.

Good questions all around, I've done some cleanup and moved the methods back out of render(), in what is hopefully the final™ commit on this pr.

LudvigHz

Nice! 🚀 🚀 🚀

eikhr

LTGM! 💯 But it seems like there are some small flow problems

eikhr

Great!

ollfkaih changed the title ~~[WIP] Add mazemap embed to events and meetings~~ Add mazemap embed to events and meetings Jan 6, 2022

ollfkaih marked this pull request as ready for review January 6, 2022 20:29

SmithPeder reviewed Jan 6, 2022

View reviewed changes

ivarnakken reviewed Jan 7, 2022

View reviewed changes

app/components/Search/mazemapAutocomplete.js Outdated Show resolved Hide resolved

ollfkaih commented Jan 9, 2022

View reviewed changes

ollfkaih requested a review from SmithPeder January 15, 2022 12:41

tmsolber and others added 12 commits January 26, 2022 10:58

Add mazemap view to events

03c8315

Add mazemap view to meetings

ddfb893

Add mazemap embed to events

b13ff17

Added search for mazemap locations

c00300a

Implemented map embed using Mazemap api (properly) for events and mee…

6882250

…tings

Allow embed height to be passed as a prop

ac03d56

Added maximum height to zoom control

a72a8f2

Bundled mazemap with webpack, made tests run

1e07286

Refactoring, renaming and less horrible colors

37b31e7

Added link on event editor and mazemap toggle

462db8f

You can now toggle between location textfield and mazemap

Use location as display name on edit forms

e4cb0ac

and moved hooks to more appropriate location

Attempted fix of directory error on ci server

74fd5a6

ollfkaih force-pushed the master branch from 9b0d7af to 74fd5a6 Compare January 26, 2022 10:01

Simplify directory structure

e74fa54

Maybe drone can find mazemap now (?)

ollfkaih requested a review from ivarnakken February 1, 2022 16:24

eikhr requested review from LudvigHz, eikhr, AdrianAndersen and danielyanghansen February 1, 2022 16:25

eikhr requested a review from juniwbjerde February 1, 2022 16:25

ivarnakken reviewed Feb 1, 2022

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

fix mazemap install

9daa09e

eikhr force-pushed the master branch from 656bc33 to 9daa09e Compare February 2, 2022 18:21

SmithPeder approved these changes Feb 2, 2022

View reviewed changes

PeterJFB approved these changes Feb 2, 2022

View reviewed changes

LudvigHz requested changes Feb 2, 2022

View reviewed changes

ollfkaih added 2 commits February 4, 2022 09:12

Fix link bug in meeting editor

facb844

Load mazemap on client (fix ssr)

188bdd2

Fix ssr bug in autocomplete field

9d2fcc2

ollfkaih force-pushed the master branch from 3dbfc44 to 9d2fcc2 Compare February 9, 2022 11:46

LudvigHz reviewed Feb 9, 2022

View reviewed changes

Dynamic mazemap imports in resonable ways

efd74d1

LudvigHz approved these changes Feb 9, 2022

View reviewed changes

eikhr reviewed Feb 9, 2022

View reviewed changes

Stupid flow

e437cc6

eikhr approved these changes Feb 9, 2022

View reviewed changes

LudvigHz merged commit 03a2012 into webkom:master Feb 9, 2022

eikhr mentioned this pull request Feb 9, 2022

[WIP] Mazemap #1680

Closed

eikhr mentioned this pull request Feb 18, 2022

Add MazeMap integration webkom/lego#1446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mazemap embed to events and meetings #2383

Add mazemap embed to events and meetings #2383

ollfkaih commented Dec 29, 2021 •

edited

SmithPeder left a comment

ollfkaih commented Jan 8, 2022

SmithPeder commented Jan 8, 2022

ollfkaih commented Jan 8, 2022

SmithPeder commented Jan 8, 2022

ollfkaih commented Jan 9, 2022

ollfkaih Jan 9, 2022

LudvigHz Feb 2, 2022

SmithPeder left a comment

PeterJFB left a comment

LudvigHz left a comment •

edited

LudvigHz Feb 2, 2022

ollfkaih commented Feb 8, 2022 via email

LudvigHz commented Feb 8, 2022

ollfkaih commented Feb 8, 2022 •

edited

LudvigHz commented Feb 8, 2022

LudvigHz Feb 9, 2022

ollfkaih Feb 9, 2022

LudvigHz left a comment

eikhr left a comment

eikhr left a comment

Add mazemap embed to events and meetings #2383

Add mazemap embed to events and meetings #2383

Conversation

ollfkaih commented Dec 29, 2021 • edited

SmithPeder left a comment

Choose a reason for hiding this comment

ollfkaih commented Jan 8, 2022

SmithPeder commented Jan 8, 2022

ollfkaih commented Jan 8, 2022

SmithPeder commented Jan 8, 2022

ollfkaih commented Jan 9, 2022

ollfkaih Jan 9, 2022

Choose a reason for hiding this comment

LudvigHz Feb 2, 2022

Choose a reason for hiding this comment

SmithPeder left a comment

Choose a reason for hiding this comment

PeterJFB left a comment

Choose a reason for hiding this comment

LudvigHz left a comment • edited

Choose a reason for hiding this comment

LudvigHz Feb 2, 2022

Choose a reason for hiding this comment

ollfkaih commented Feb 8, 2022 via email

LudvigHz commented Feb 8, 2022

ollfkaih commented Feb 8, 2022 • edited

LudvigHz commented Feb 8, 2022

LudvigHz Feb 9, 2022

Choose a reason for hiding this comment

ollfkaih Feb 9, 2022

Choose a reason for hiding this comment

LudvigHz left a comment

Choose a reason for hiding this comment

eikhr left a comment

Choose a reason for hiding this comment

eikhr left a comment

Choose a reason for hiding this comment

ollfkaih commented Dec 29, 2021 •

edited

LudvigHz left a comment •

edited

ollfkaih commented Feb 8, 2022 •

edited