-
Notifications
You must be signed in to change notification settings - Fork 189
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
Create onlyoffice extension #857
Conversation
This comment has been minimized.
This comment has been minimized.
43dd342
to
75e41b4
Compare
Just realising now. I've actually added the extension as default into the config.json in ocis-phoenix which doesn't make sense because it won't work yet in pure ocis setup. Need to think about how do we want to deliver this in bridge setup. |
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 discovered some files that are not needed, some files that need to be one folder level higher, some occurrences of ocis-onlyoffice
and some code that can be deleted.
Also, please contact @C0rby on how to implement the version command here and how to add it to the boilr template, so that we have it in every new service.
Good to see this extension! Really nice progress.
21b7641
to
79ec1d1
Compare
bff7734
to
6afa02e
Compare
@kulmann I've addressed all your comments. Regarding the versions command, I would like to tackle that in separate PR to finally get this one merged. |
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.
Fix the staticcheck, then it's good to go 🚀
Kudos, SonarCloud Quality Gate passed! |
Requires owncloud/web#4324 to work in ocis-web. Testing requires running oc10 server with the onlyoffice app installed and enabled + running onlyoffice docs server.
Some summary of my brain trips when creating this:
Originally I was considering creating a component which would contain an iframe loading the editor so we are not really leaving ocis-web. This had two problems though - we would need to set a new header to allow this and we would anyway see the topbar of oc10. That's why I decided to go the way of opening the editor in a new tab.
Regarding tests - I don't really see any value in tests here. We do not have any views, any big logic... If the handler is working should be tested in ocis-web on a more general level. Testing if
window.open
seems like a needless increase of CI time.