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

Modal for roomfinder #281

Merged
merged 15 commits into from Dec 6, 2022
Merged

Conversation

Marius1501
Copy link
Contributor

@Marius1501 Marius1501 commented Nov 8, 2022

Resolves: #143
grafik

Is the roomfinder-map sufficient or should the map-cross also be implemented to the modal?
I already tried to implement the cross but it is a bit difficult with the position because I think you have to make a function like the loadRoomfinderMap-function for the roomfinder modal.

@octycs
Copy link
Contributor

octycs commented Nov 9, 2022

I already tried to implement the cross but it is a bit difficult with the position because I think you have to make a function like the loadRoomfinderMap-function for the roomfinder modal.

It can probably be simplified a bit for the modal, because not all parts are relevant (I've not tested this code, but this is what I think is relevant):

 loadModalRoomfinderMap: function () {
      const map = this.view_data.maps.roomfinder.available[mapIndex];

      const rect = document
        .getElementById("TBD")
        .getBoundingClientRect();
      // -1023px, -1023px is top left corner, 16px = 2*8px is element padding
      this.state.map.roomfinder.modalX =
        -1023 + (map.x / map.width) * (rect.width - 16);

      // We cannot use "height" here as it might be still zero before layouting
      // finished, so we use the aspect ratio here.
      this.state.map.roomfinder.modalY =
        -1023 +
        (map.y / map.height) * (rect.width - 16) * (map.height / map.width);
}

But one problem may be that the HTML elements will not yet be in the DOM if you put that in the click handler at the moment. Because the v-if here will only insert the modal HTML elements once map.roomfinder.modalRoomfinder_open is true, and (as far I remember) all handler code completed. But in order to compute the position of the cross, the element would need to be in the DOM already so you can get the dimensions and compute the cross position.

One way to fix that could be to toggle only the active class of the modal using map.roomfinder.modalRoomfinder_open (v-bind:class="{ active: map.roomfinder.modalRoomfinder_open }") and leave of the v-if. Then the modal is in the DOM all the time (not ideal, but because it's inactive probably not too bad).

(Maybe it's better to call it map.roomfinder.modalOpen, then roomfinder is not duplicate and it's shorter)

On the other hand, I would also be okay to merge without the cross implemented, I don't know how much difference it makes in terms of the Vue3 port @CommanderStorm ?

@CommanderStorm
Copy link
Member

On the other hand, I would also be okay to merge without the cross implemented, I don't know how much difference it makes in terms of the Vue3 port @CommanderStorm ?

I am indifferent about this. Migrating this should be quite straightforward (I cannot guarantee commit ownership though, as all commits in #200 will have to be squashed before merging into main..)

@CommanderStorm CommanderStorm added the frontend Related to the frontend label Nov 12, 2022
@Marius1501
Copy link
Contributor Author

@octycs did you think of this change or did I something wrong? Because it is not working properly. The modal is now open when I leave out the v-if. And the cross is now in the left upper corner. Maybe the loadModalRoomfinder function is called in the wrong place. I thought it would be appropriate to call the function when you click on the roomfinder-map.

@octycs
Copy link
Contributor

octycs commented Nov 14, 2022

Did you add the loadModalRoomfinderMap function into view-view.js? I can't see it in the diff

@Marius1501
Copy link
Contributor Author

Oh sorry. I tried the code in another directory and forgot to add the loadModalRoomfinderMap function.

@Marius1501
Copy link
Contributor Author

@octycs Now the positioning of the cross should be right. But I didn't get rid of the overflow of the cross.

@Marius1501
Copy link
Contributor Author

Ok, thank you for your help! Now everything should be right from my perspective. 😄

@Marius1501 Marius1501 marked this pull request as ready for review November 28, 2022 19:48
Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Other than the timeout and other minor questions I think this is nearly ready to merge.

One thing: This chip is important for legal reasons
Please either add a chip like this one or add a bottom row like with the images 😉
image
image

webclient/src/views/view/view-view.inc Outdated Show resolved Hide resolved
webclient/src/views/view/view-view.js Show resolved Hide resolved
webclient/src/views/view/view-view.scss Outdated Show resolved Hide resolved
webclient/src/views/view/view-view.scss Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm marked this pull request as draft November 28, 2022 23:54
@Marius1501 Marius1501 marked this pull request as ready for review November 29, 2022 08:19
@CommanderStorm CommanderStorm merged commit d2b2a1d into TUM-Dev:main Dec 6, 2022
@CommanderStorm
Copy link
Member

Sorry for the long wait 😅

@Marius1501 Marius1501 deleted the modal_for_roomfinder branch December 6, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Related to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Site Maps Zoom
3 participants