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

feat:select existing talk room for conversations on calendar events #6595

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

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Dec 30, 2024

fixes #6581

This PR adds the ability to link Talk rooms to calendar events. Users can create a new Talk room or select an existing one, and the link is added to the event's location or description(when the location field is already filled).
How to test:

  1. Create an event on calendar
  2. Open the side bar by clicking more details
  3. Find the Add Talk button

hover state and selected state
Screenshot from 2025-01-08 12-12-10

the side bar after selecting an existing convesation
Screenshot from 2025-01-08 17-17-38

Screenshot from 2025-01-08 17-15-40

@GretaD GretaD added the 2. developing Work in progress label Dec 30, 2024
@GretaD GretaD self-assigned this Dec 30, 2024
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 100 lines in your changes missing coverage. Please review.

Project coverage is 23.18%. Comparing base (8f9e504) to head (b8396ef).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/components/Editor/AddTalkModal.vue 0.00% 77 Missing ⚠️
src/views/EditSidebar.vue 0.00% 22 Missing ⚠️
src/services/talkService.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6595      +/-   ##
============================================
- Coverage     23.41%   23.18%   -0.24%     
  Complexity      472      472              
============================================
  Files           249      250       +1     
  Lines         11899    12018     +119     
  Branches       2269     2308      +39     
============================================
  Hits           2786     2786              
- Misses         8786     8905     +119     
  Partials        327      327              
Flag Coverage Δ
javascript 14.67% <0.00%> (-0.19%) ⬇️
php 59.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GretaD GretaD changed the title feat:select existing talk roomfor convesations feat:select existing talk room for conversations on calendar events Dec 30, 2024
@nimishavijay
Copy link
Member

nimishavijay commented Jan 7, 2025

Changes discussed:

  • Nice-to-have: avatar of the each conversation (34*34px)
  • Instead of a "Select" button for each item, have only one select button in the bottom right corner
  • When you click on an item, it will be highlighted in the primary color (similar to the Talk conversation list)

Additional changes:

  • heading wording "Select a talk room from the list" --> "Add Talk conversation"
  • use small modal size
  • Nice-to-have: sort by alphabetical order

Follow ups:

  • Show conversation description in subline
  • Add search bar which filters the list

@st3iny
Copy link
Member

st3iny commented Jan 7, 2025

For reference, the URL for getting conversation avatars is the following:

const avatarUrl = generateOcsUrl('apps/spreed/api/v1/room/{roomToken}/avatar', {
	roomToken: conversation.token,
})

@GretaD GretaD force-pushed the feat/add-talk-calendar branch from 1153693 to d9749df Compare January 8, 2025 10:27
@GretaD GretaD marked this pull request as ready for review January 8, 2025 10:28
@GretaD GretaD added 3. to review Waiting for reviews enhancement New feature request and removed 2. developing Work in progress labels Jan 8, 2025
src/components/Editor/AddTalkModal.vue Outdated Show resolved Hide resolved
src/components/Editor/AddTalkModal.vue Outdated Show resolved Hide resolved
src/components/Editor/AddTalkModal.vue Outdated Show resolved Hide resolved
@nimishavijay
Copy link
Member

Looks awesome! Could we try with 28px size for the avatar and 34px height of the whole element?

I would also suggest to add some spacing between the avatar and the conversation name, maybe one more --default-grid-baseline :)

@GretaD
Copy link
Contributor Author

GretaD commented Jan 8, 2025

Looks awesome! Could we try with 28px size for the avatar and 34px height of the whole element?

sure

I would also suggest to add some spacing between the avatar and the conversation name, maybe one more --default-grid-baseline :)

--default-grid-baseline is 4px, i have added 5px, so how big should i make it?

Agreed with nimisha to have it 8px

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Looks great design wise 🚀

@tcitworld
Copy link
Member

I'm not fond of
image

The location field is also used for readable information when it's a physical location, not just URLs, and restricting the width again makes it harder to do modifications to a physical address.
Maybe but the button below on subline?

@GretaD
Copy link
Contributor Author

GretaD commented Jan 8, 2025

I'm not fond of image

The location field is also used for readable information when it's a physical location, not just URLs, and restricting the width again makes it harder to do modifications to a physical address. Maybe but the button below on subline?

@nimishavijay what do you think?

@nimishavijay

This comment was marked as outdated.

@GretaD

This comment was marked as outdated.

@tcitworld
Copy link
Member

Yup, looks good!

@GretaD
Copy link
Contributor Author

GretaD commented Jan 8, 2025

changed the desc with the latest state

@GretaD
Copy link
Contributor Author

GretaD commented Jan 8, 2025

Yup, looks good!

thank you, changed the position as suggested by nimisha. Can you please approve if you are happy with it?

@tcitworld
Copy link
Member

changed the position as suggested by nimisha

The button is above the field instead of under it? No opinion on this, but it differs with your previous screenshot.

@GretaD
Copy link
Contributor Author

GretaD commented Jan 8, 2025

changed the position as suggested by nimisha

The button is above the field instead of under it? No opinion on this, but it differs with your previous screenshot.

yes, she sent me a mockup. Sorry if i was not clear. i marked my comment as outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select existing Talk room for conversation, and streamline creation of new one
5 participants