-
Notifications
You must be signed in to change notification settings - Fork 12
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(actions): standardize contexts #1124
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: skjnldsv <[email protected]>
Bundle ReportBundle size has no change ✅ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1124 +/- ##
=======================================
Coverage 91.62% 91.62%
=======================================
Files 23 23
Lines 633 633
Branches 166 166
=======================================
Hits 580 580
Misses 46 46
Partials 7 7 ☔ View full report in Codecov by Sentry. |
I would try to make the library to work with all supported Nextcloud versions. What I mean is we can do it like in this PR, which would be breaking and a v4 of the library. So how to do it while not breaking server API? This way an app using files v3 work with 28,29,30,31 as per our deprecation rules (we had this discussion with management at the Nextcloud conference). Context: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 1 or 2 parameters before a context object in a function declaration does not seem that bad to me IMHO, any reason why we should never do this @skjnldsv?
import { View } from './navigation' | ||
|
||
type ActionContextSingle = { | ||
nodes: [Node], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply
nodes: [Node], | |
node: Node, |
?
type ActionContextSingle = { | ||
nodes: [Node], | ||
view: View, | ||
context: Folder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context: Folder, | |
folder: Folder, |
Calling the folder context
inside the context object seems confusing to me 😵
We should use "context" only for context objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's to do a clear separation between the nodes. So we know folder isn't the subject of this action
Because it just looks weird to me. From a clean declaration perspective, I would agree with But mixing something like But more importantly, I like to be consistent across our APIs here, and considering we have multiple APIs that do similar approaches, like FileAction, NewFileMenu, Views..., I think we should stick to one type of pattern here. That way it's easy to know what to expect when you work with the |
Following discussion on #1113
This would be a breaking change unless we add a conversion layer to support the old legacy way.
BUT, the cleanest way, imho, is to quickly update server and communicate to devs asap