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.

Sign up for GitHub

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

Jump to bottom

feat(files): support nested actions #41010

Merged
merged 4 commits into from Nov 8, 2023
Merged

feat(files): support nested actions #41010

merged 4 commits into from Nov 8, 2023

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 20, 2023 •
edited

@skjnldsv skjnldsv added this to In progress in Files to vue via automation Oct 20, 2023
@skjnldsv skjnldsv self-assigned this Oct 20, 2023
@skjnldsv skjnldsv mentioned this pull request Oct 20, 2023
Merged
@skjnldsv skjnldsv requested a review from szaimen October 20, 2023 10:40
skjnldsv
skjnldsv commented Oct 20, 2023
View reviewed changes
apps/files/src/init.ts Outdated Show resolved Hide resolved
@skjnldsv skjnldsv marked this pull request as draft October 20, 2023 10:40
@szaimen

This comment was marked as resolved.

Sign in to view
@skjnldsv

This comment was marked as resolved.

Sign in to view
@szaimen

This comment was marked as resolved.

Sign in to view
Pytal

This comment was marked as resolved.

Sign in to view

@skjnldsv

This comment was marked as resolved.

Sign in to view
marcoambrosini

This comment was marked as off-topic.

Sign in to view

@skjnldsv

This comment was marked as resolved.

Sign in to view
@marcoambrosini marcoambrosini mentioned this pull request Oct 25, 2023
Closed
@nimishavijay

This comment was marked as resolved.

Sign in to view
@skjnldsv

This comment was marked as resolved.

Sign in to view
@Pytal

This comment was marked as resolved.

Sign in to view
@solracsf solracsf added this to the Nextcloud 28 milestone Oct 27, 2023
This was referenced Oct 31, 2023
Merged
Merged
jancborchardt

This comment was marked as off-topic.

Sign in to view

@jancborchardt

This comment was marked as resolved.

Sign in to view
@skjnldsv skjnldsv marked this pull request as ready for review November 3, 2023 17:56
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 3, 2023 •
edited

@Pytal please review the logic!

Especially the condition on where a reminder is shown or not, as I wasn't part of the original discussions! I used your services, so I think it should be good.
But here are a few questions to be safe

  1. Is it normal that depending on the time of day, only some options are visible?
  2. Is this feature enabled for all nodes, no matter what they are and what permissions they have? (folder/files, etc etc)

@Pytal
Copy link
Member

Pytal commented Nov 4, 2023

  1. Is it normal that depending on the time of day, only some options are visible?

Yes this is by design

  1. Is this feature enabled for all nodes, no matter what they are and what permissions they have? (folder/files, etc etc)

Yes, if you can see (read) the file then you should be able to set a reminder on it

Pytal
Pytal reviewed Nov 4, 2023
View reviewed changes
apps/files/src/components/FileEntry/FileEntryActions.vue Outdated Show resolved Hide resolved
apps/files/src/components/FileEntry/FileEntryActions.vue Outdated Show resolved Hide resolved
apps/files_reminders/src/actions/setReminderSuggestionActions.scss Outdated Show resolved Hide resolved
apps/files_reminders/src/actions/setReminderSuggestionActions.ts Show resolved Hide resolved
apps/files_reminders/src/actions/setReminderSuggestionActions.ts Outdated Show resolved Hide resolved
apps/files_reminders/src/actions/setReminderSuggestionActions.ts Outdated Show resolved Hide resolved
apps/files_reminders/src/components/SetCustomReminderModal.vue Outdated Show resolved Hide resolved
apps/files_reminders/src/services/customPicker.ts Outdated Show resolved Hide resolved
@blizzz blizzz mentioned this pull request Nov 6, 2023
Merged
@skjnldsv skjnldsv mentioned this pull request Nov 7, 2023
Open
@skjnldsv skjnldsv requested a review from Pytal November 7, 2023 10:41
susnux
susnux requested changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

The focus handling does not work properly:
If you click (keyboard) on "set reminder" the submenu opens but the focus is not set on the submenu's first element.

Also if you click the back button the first submenu is reopend, but the focus is not on the "set reminder" entry.

In both cases the focus is on some random non visible element.

Pytal
Pytal reviewed Nov 7, 2023
View reviewed changes
apps/files_reminders/src/actions/setReminderSuggestionActions.ts
}

// Generate the default preset actions
export const actions = [laterToday, tomorrow, thisWeekend, nextWeek]
Copy link
Member

Choose a reason for hiding this comment

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

Since the presence of the action should be evaluated ideally on click instead of on script execution I think the FileAction enabled callback needs to be set to something like () => Boolean(getDateTime(option.dateTimePreset)) to be more reactive

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be improved later

susnux
susnux approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

🐘 a11y in follow up?

@skjnldsv
feat(files): support nested actions
72ffd49 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
chore(files_reminders): upgrade to 28 APIs
cf6c921 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
chore(jest): update snapshot after component update
50417bc 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
fix(cypress): formatFileSize change revert from files library
32c1aeb 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv merged commit 069eb07 into master Nov 8, 2023
48 of 49 checks passed
Files to vue automation moved this from In progress to Done Nov 8, 2023
@skjnldsv skjnldsv deleted the feat/nested-actins branch November 8, 2023 07:53
This was referenced Nov 8, 2023
Merged
Closed
@max-nextcloud max-nextcloud mentioned this pull request Dec 15, 2023
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jancborchardt jancborchardt jancborchardt left review comments

@marcoambrosini marcoambrosini marcoambrosini left review comments

@susnux susnux susnux approved these changes

@Pytal Pytal Pytal approved these changes

@szaimen szaimen Awaiting requested review from szaimen

@artonge artonge Awaiting requested review from artonge

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews enhancement feature: files
Projects
Files to vue
  
Done
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

Add API for FileAction submenu actions
8 participants
@skjnldsv @szaimen @nimishavijay @Pytal @jancborchardt @susnux @marcoambrosini @solracsf