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

Expose room location in room booking's UI #5622

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

mindouro
Copy link
Contributor

@mindouro mindouro commented Jan 12, 2023

Closes #4291

This PR adds the possibility to filter by location in the Room Booking, when there are more than one location.
This also adds the location value in the room's card and inside the room's details modal.

SCR-20230112-ehz
SCR-20230112-eig
SCR-20230112-ejl

@ThiefMaster
Copy link
Member

Nice! I wonder if the Location filter should be shown only if there's more than one location, since otherwise it's pretty pointless to filter by it.

@mindouro
Copy link
Contributor Author

Nice! I wonder if the Location filter should be shown only if there's more than one location, since otherwise it's pretty pointless to filter by it.

I'm checking how many locations we have before showing the location's filter to avoid that. I also won't show the location in the room's card or details if there is only one location. 🙂

@mindouro mindouro marked this pull request as ready for review January 12, 2023 11:09
CHANGES.rst Outdated Show resolved Hide resolved
indico/modules/rb/client/js/common/rooms/selectors.js Outdated Show resolved Hide resolved
indico/modules/rb/client/js/common/rooms/selectors.js Outdated Show resolved Hide resolved
@mindouro mindouro force-pushed the expose-location-in-ui branch 2 times, most recently from 39a95e0 to c8e1890 Compare January 17, 2023 18:35
Copy link
Member

@ThiefMaster ThiefMaster left a comment

Choose a reason for hiding this comment

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

Very nice PR! Just a few small fixes and we can merge this. :)

CHANGES.rst Outdated Show resolved Hide resolved
Comment on lines 183 to 187
{showLocation && (
<div>
<Icon name="map pin" /> {room.locationName || Translate.string('Not specified')}
</div>
)}
Copy link
Member

Choose a reason for hiding this comment

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

The room card looks rather broken with the location information added:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens because the class .room-card in Room.module.scss has a fixed height value of 320px. My solution was removing the height of it and let Semantic UI's Card.Group component manage the card's heigh, keeping the same height for all cards of that row and avoiding future problems when adding any other new fields to the room card. Let me know if you prefer other approach 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really convinced by the new look TBH. It still looks "wrong".
Maybe we can just ditch showing the location in the room card? It's not very important and if you want to know it you can either filter by it or open the room details...

Copy link
Member

Choose a reason for hiding this comment

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

(I added a commit to not show the location there)

It's not THAT important to show it there and it usually results in weird
layout (even more so if it's a long location name)
@ThiefMaster ThiefMaster added this to the v3.2 milestone Feb 2, 2023
@ThiefMaster ThiefMaster merged commit d28ec19 into indico:master Feb 2, 2023
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.

Room Booking: Expose Location in the UI
2 participants