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

Make the viewer standalone #271

Merged
merged 6 commits into from
Nov 5, 2019
Merged

Make the viewer standalone #271

merged 6 commits into from
Nov 5, 2019

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 25, 2019

Please have a look!

This is the first throw at making the Viewer standalone and implementing a way to call it directly.

OCA.Viewer.open('/your/path/to/image.jpg')
OCA.Viewer.close()

It is also a first throw at implementing a way to automatically build a FileInfo object. Please have a look at

const genFileInfo = function(obj) {
const fileInfo = {}
Object.keys(obj).forEach(key => {
const data = obj[key]
// flatten object if any
if (typeof data === 'object') {
Object.assign(fileInfo, genFileInfo(data))
} else {
// format key and add it to the fileInfo
if (data === 'false') {
fileInfo[camelcase(key)] = false
} else if (data === 'true') {
fileInfo[camelcase(key)] = true
} else {
fileInfo[camelcase(key)] = isNumber(data)
? Number(data)
: data
}
}
})
return fileInfo
}

  • Use user saved sorting key

@skjnldsv skjnldsv added enhancement New feature or request high High priority 3. to review Waiting for reviews integration Compatibility with other apps labels Oct 25, 2019
@skjnldsv skjnldsv self-assigned this Oct 25, 2019
@skjnldsv skjnldsv requested a review from rullzer October 25, 2019 17:02
@cypress
Copy link

cypress bot commented Oct 25, 2019



Test summary

96 0 0 0


Run details

Project viewer
Status Passed
Commit 0821eb9
Started Nov 5, 2019 10:39 PM
Ended Nov 5, 2019 10:40 PM
Duration 00:54 💡
OS Linux Debian - 8.11
Browser Electron 61

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@skjnldsv skjnldsv force-pushed the refactor/standalone branch 3 times, most recently from ccbbaa1 to 7650c35 Compare October 26, 2019 08:30
@juliusknorr
Copy link
Member

juliusknorr commented Oct 28, 2019

OCA.Viewer.file = '/your/path/to/image.jpg'

Any reason you make this a property and not just methods to show the viewer with a given path? Feels a bit un-intuitive to me.

Other questions:

  • Is this supposted to also work with relative paths on public share links?
  • Would the viewer still automatically fetch the parent directory so the users can go though all pictues?

@skjnldsv skjnldsv force-pushed the refactor/standalone branch from 47a9d43 to 7b8b409 Compare October 28, 2019 08:36
@skjnldsv
Copy link
Member Author

Any reason you make this a property and not just methods to show the viewer with a given path? Feels a bit un-intuitive to me.

Ah, Roeland thought the same! :)

Well, this is how I implemented it on the other apis I created, like the sidebar.
It's a class getter/setter. My argument was that apps could want to get the current opened file, so they'll do OCA.Viewer.file. Therefore it felt weird to have one function to set the file OCA.Viewer.openFile(path) and OCA.Viewer.file to get the current one.

How would you have wanted it? 🤔

@juliusknorr
Copy link
Member

How would you have wanted it?

It's just that this is nothing I've seen being done so far in javascript APIs that are somehow "public". 😉

Not sure if that is really needed anywhere but with a proper method the open call could return a promise that resolves when the viewer is properly loaded.

@skjnldsv
Copy link
Member Author

Not sure if that is really needed anywhere but with a proper method the open call could return a promise that resolves when the viewer is properly loaded.

Well, I could just expose the openFile method from within the vue component yes :)
Would you find it better?

@skjnldsv
Copy link
Member Author

@juliushaertl can you test text with this branch ? :)

@juliusknorr
Copy link
Member

juliusknorr commented Oct 28, 2019

@juliushaertl can you test text with this branch ? :)

Still works as expected, with one issue:

  • the sidebar is missing (server on master)/ throwing the following errors (server on the new sidebar branch):
vue.runtime.esm.js:619 [Vue warn]: Error in v-on handler: "TypeError: Cannot read property 'offsetWidth' of null"

found in

---> <Actions>
       <Modal>
         <Viewer> at src/views/Viewer.vue
           <Root>
warn @ vue.runtime.esm.js:619
logError @ vue.runtime.esm.js:1884
globalHandleError @ vue.runtime.esm.js:1879
handleError @ vue.runtime.esm.js:1839
invokeWithErrorHandling @ vue.runtime.esm.js:1862
invoker @ vue.runtime.esm.js:2179
original._wrapper @ vue.runtime.esm.js:6911
vue.runtime.esm.js:1888 TypeError: Cannot read property 'offsetWidth' of null
    at VueComponent.showAppsSidebar (Viewer.vue?a0e8:613)
    at VueComponent.showSidebar (Viewer.vue?a0e8:592)
    at VueComponent.execFirstAction (Modal.js:112)
    at invokeWithErrorHandling (vue.runtime.esm.js:1854)
    at HTMLButtonElement.invoker (vue.runtime.esm.js:2179)
    at HTMLButtonElement.original._wrapper (vue.runtime.esm.js:6911)
logError @ vue.runtime.esm.js:1888
globalHandleError @ vue.runtime.esm.js:1879
handleError @ vue.runtime.esm.js:1839
invokeWithErrorHandling @ vue.runtime.esm.js:1862
invoker @ vue.runtime.esm.js:2179
original._wrapper @ vue.runtime.esm.js:6911

@skjnldsv
Copy link
Member Author

  • the sidebar is missing (server on master)/ throwing the following errors (server on the new sidebar branch):

The button is not supposed to be shown if the sidebar is not here 🤔

v-if="OCA.Files && OCA.Files.Sidebar"

How did you triggered this error?

@juliusknorr
Copy link
Member

The button is not supposed to be shown if the sidebar is not here thinking

Yes the button is missing if the server is on master, the error happened with the server on the new sidebar branch when clicking the sidebar button.

@skjnldsv
Copy link
Member Author

Yes the button is missing if the server is on master, the error happened with the server on the new sidebar branch when clicking the sidebar button.

ah yes! Indeed, this is not compatible with the new sidebar yet :)

appinfo/app.php Outdated Show resolved Hide resolved
lib/AppInfo/Application.php Outdated Show resolved Hide resolved
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 4, 2019

/compile amend /

@skjnldsv skjnldsv force-pushed the refactor/standalone branch 3 times, most recently from c7bb566 to 8ada0f6 Compare November 5, 2019 16:58
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the refactor/standalone branch from 8ada0f6 to f3a1011 Compare November 5, 2019 17:08
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the refactor/standalone branch 7 times, most recently from 798745a to abe8d31 Compare November 5, 2019 22:21
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the refactor/standalone branch from abe8d31 to 0821eb9 Compare November 5, 2019 22:38
@skjnldsv skjnldsv merged commit c76f79a into master Nov 5, 2019
@skjnldsv skjnldsv deleted the refactor/standalone branch November 5, 2019 22:44
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
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 or request high High priority integration Compatibility with other apps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants