-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add filepicker filter #577
Conversation
@PVince81 any questions about this that we need to discuss? |
|
||
public constructor(title: string, | ||
multiSelect: boolean, | ||
mimeTypeFilter: string[], | ||
modal: boolean, | ||
type: FilePickerType, | ||
directoriesAllowed: boolean, | ||
path?: string) { | ||
path?: string, | ||
filter?: Nextcloud.v24.FilePickerFilter) { |
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.
am not familiar enough with Typescript to be able to decide whether it's a good idea to define the filter this way
@ChristophWurst can you confirm ?
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.
Right. That would wrongly indicate that the argument is available for 22 and 23 as well, doesn't it?
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.
I believe @PVince81 is referring to the Nextcloud.v24.FilePickerFilter
type which makes sense to me as it's definition and functionality is stable as per nextcloud/server#31972 introduced in Nextcloud 24, otherwise I guess we could have a Nextcloud.Stable namespace in https://github.com/nextcloud/nextcloud-typings but is maybe not worth the upkeep
I'd suggest we just bump the major version of this package when releasing, @ChristophWurst @PVince81 thoughts?
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.
Yep, should be a major version
What is the status here? |
As stated above #577 (comment) I'm fine with bumping the major and releasing so good from my side but I can't approve as I opened this :) Look good @PVince81 @ChristophWurst? |
f65d2d7
to
466bbd3
Compare
Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
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.
👍
Close #560