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

Move resources into Details tab and add new Media tab with related resources panel #4498

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

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Sep 10, 2022

Description Screenshot
Related resources image
No related resources image

Event resources

Before (Resources tab) After (Moved into Details tab)
image image

Related

@codecov
Copy link

codecov bot commented Sep 10, 2022

Codecov Report

Base: 41.30% // Head: 41.82% // Increases project coverage by +0.51% 🎉

Coverage data is based on head (854c7b4) compared to base (9e8acdf).
Patch coverage: 33.33% of modified lines in pull request are covered.

❗ Current head 854c7b4 differs from pull request most recent head 56649e7. Consider uploading reports for the commit 56649e7 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4498      +/-   ##
============================================
+ Coverage     41.30%   41.82%   +0.51%     
- Complexity      339      348       +9     
============================================
  Files           226      226              
  Lines          5675     5662      -13     
  Branches        743      738       -5     
============================================
+ Hits           2344     2368      +24     
+ Misses         3331     3294      -37     
Flag Coverage Δ
javascript 31.42% <33.33%> (+0.26%) ⬆️
php 69.05% <ø> (+0.57%) ⬆️

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

Impacted Files Coverage Δ
src/views/EditSidebar.vue 0.00% <0.00%> (ø)
src/models/calendar.js 100.00% <100.00%> (ø)
...ud/apps/calendar/lib/Controller/ViewController.php 80.48% <0.00%> (-0.47%) ⬇️
src/store/settings.js 94.23% <0.00%> (-0.06%) ⬇️
src/store/calendars.js 0.00% <0.00%> (ø)
src/views/Calendar.vue 0.00% <0.00%> (ø)
src/store/calendarObjectInstance.js 0.00% <0.00%> (ø)
...nts/AppNavigation/CalendarList/CalendarListNew.vue 0.00% <0.00%> (ø)
...ts/AppNavigation/CalendarList/CalendarListItem.vue 0.00% <0.00%> (ø)
...s/calendar/lib/Controller/PublicViewController.php 100.00% <0.00%> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tcitworld
Copy link
Member

So the resources are related to the calendar, not the event?

Also, if the tab is named « Media » people might expect upcoming event attachments to be here too.

@Pytal
Copy link
Member Author

Pytal commented Sep 13, 2022

So the resources are related to the calendar, not the event?

Yes that is how it is implemented, please confirm @ArtificialOwl

Also, if the tab is named « Media » people might expect upcoming event attachments to be here too.

Will move the attachments as well

src/views/EditSidebar.vue Outdated Show resolved Hide resolved
@Pytal Pytal force-pushed the enh/related-resources branch 3 times, most recently from 854c7b4 to 483e358 Compare October 29, 2022 01:11
@Pytal Pytal added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 29, 2022
@Pytal
Copy link
Member Author

Pytal commented Nov 30, 2022

Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal marked this pull request as ready for review December 2, 2022 02:04
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Member

@ChristophWurst can you help move this forward ?

@PVince81
Copy link
Member

@ChristophWurst @miaulalala @GretaD needs second review

@miaulalala
Copy link
Contributor

Sorry I really dislike the look of this feature.

Users are used to the separation of a tab for resources, and having resources in the Details view doesn't make sense.

We added the resources as a separate tab for a reason.

Why not add a 4th tab such as in the linked PR?

@PVince81
Copy link
Member

@jancborchardt @nimishavijay ^

@tcitworld
Copy link
Member

I also have some concerns with the whole thing:

  • From what I can see in the code, related resources are opened in the same tab by default, so people will lose their event changes if they click on them before saving the event.
  • There's already little share of users using the current resources tab, due to the lack of a GUI to manage resources and the fact only admins can manage them. Moving those into the Details first tab which is already really quite full since reminders don't have their tab anymore will only confuse users more.
  • The fact that we show media in the event editor, whereas relations are made through calendars, makes little sense. Calendar shares don't change often, so my understanding is that users might always see pretty much the same items in this section
  • From other apps, since there's no way to "view" a calendar in particular, clicking on a related resource with is a calendar will pretty much only load the calendar app, the benefit is not comparable to files, talk and deck where you access the related entity directly
  • These two remarks make me think relations should really be made with calendar events (while considering things like calendar sharees, event attendees, title and date). Links from outside can then load the event directly.

Sorry for the late review, and please forgive me for being so negative on the topic, but I hope perhaps you'll find some constructive stuff in what I've said.

@PVince81
Copy link
Member

Why not add a 4th tab such as in the linked PR?

from what I heard it was an explicit decision of @jancborchardt to put it there

@miaulalala @ChristophWurst if a different location needs to be used, please take over and make the change

an alternative would be to first merge this as is and move it in a separate iteration

@miaulalala
Copy link
Contributor

@nextcloud/designers pls add your input

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.

None yet

5 participants