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: add conference field #5381

Closed
wants to merge 2 commits into from
Closed

feat: add conference field #5381

wants to merge 2 commits into from

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Jul 27, 2023

Partially addresses #1758

Simple Editor

RW RO
grafik grafik

Sidebar Editor

RW RO
grafik grafik

Signed-off-by: Richard Steinmetz <[email protected]>
@st3iny st3iny added 3. to review Waiting for reviews enhancement New feature request Feature: Editor labels Jul 27, 2023
@st3iny st3iny added this to the v4.5.0 milestone Jul 27, 2023
@st3iny st3iny self-assigned this Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +42.71% 🎉

Comparison is base (9f53311) 22.71% compared to head (a8511fc) 65.42%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #5381       +/-   ##
=============================================
+ Coverage     22.71%   65.42%   +42.71%     
  Complexity      372      372               
=============================================
  Files           237       36      -201     
  Lines         11679     1990     -9689     
  Branches       2270        0     -2270     
=============================================
- Hits           2653     1302     -1351     
+ Misses         9026      688     -8338     
Flag Coverage Δ
javascript ?
php 65.42% <ø> (ø)

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

see 201 files with indirect coverage changes

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

@tcitworld
Copy link
Member

Did you test the support of the CONFERENCE property with various clients? We might want to keep it in the description as well if it's not good enough.

@st3iny
Copy link
Member Author

st3iny commented Jul 28, 2023

Good point. I just tested it with Thunderbird and it has no support for the field. I guess we should keep the link in the description for now.

@st3iny
Copy link
Member Author

st3iny commented Jul 31, 2023

We did some testing on Microsoft (Outlook) and Mac OS (Apple Calendar) and it turns out that both clients don't support the field.

The Apple Calendar on Mac has a shared location and conference field that isn't populated from the CONFERENCE field. Manually setting it by scheduling e.g. a Zoom meeting from the client and then exporting the event shows that the link is written into the description field.

Maybe we should use the location field for the link, given that the CONFERENCE field does not seem to be supported well.

@tcitworld
Copy link
Member

Maybe we should use the location field for the link, given that the CONFERENCE field does not seem to be supported well.

#2070 has some discussions about that.

Biggest issue is that we (and other 3rd-party apps) then need to handle when the user wants to enter a physical location.

In any case, even if it's not supported anyway, it doesn't hurt to have the CONFERENCE property properly initialized. Support may come on other providers at some point.

@schiessle
Copy link
Member

schiessle commented Jul 31, 2023

Biggest issue is that we (and other 3rd-party apps) then need to handle when the user wants to enter a physical location.

I see the point, but in 90% of all use cases I like it to have the link in the location field. That's also how I send my manually created invitation at the moment. This is easy to find and most calendars offer a way to directly click it on pop-up, etc. I hate this Teams/Google invitations with lots of text, often multiple links and you have to search for the right one.

A argument for the description might be if we want to send more information, e.g. also the dial-in number if the SIP bridge is enabled.

Maybe a good solution would be to pre-file the URL in both fields, description and location? It doesn't hurt because different people might look at different fields first to find the link and it gives the creator of the event the freedom to overwrite each field if they wish without losing the link?

I agree, that we should not introduce a field which can't be handled by all/most calendars correctly.

@st3iny
Copy link
Member Author

st3iny commented Aug 1, 2023

We discussed this in the team and came up with the following plan:

  1. Write the conference URL into the location field.
  2. Implement the clickable link thingy for the location field, too.
  3. Render the location field as a clickable link in the appointment notification mail (if it contains a link).

@ChristophWurst
Copy link
Member

@st3iny todos moved into #5389. Feel free to either change this branch/PR to drop the conference and direct everything towards the new goals or start a new PR.

@st3iny
Copy link
Member Author

st3iny commented Aug 7, 2023

I'll create a new branch and PR.

@st3iny st3iny closed this Aug 7, 2023
@st3iny st3iny deleted the feat/add-conference-field branch August 7, 2023 10:34
@st3iny st3iny removed this from the v4.5.0 milestone Aug 7, 2023
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 Feature: Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants