Skip to content

Sheets Collab#122

Merged
hannessolo merged 9 commits intomainfrom
i701
Feb 26, 2026
Merged

Sheets Collab#122
hannessolo merged 9 commits intomainfrom
i701

Conversation

@hannessolo
Copy link
Contributor

Add support for collab on sheets

@hannessolo hannessolo marked this pull request as ready for review February 25, 2026 18:34
@hannessolo
Copy link
Contributor Author

hannessolo commented Feb 25, 2026

Tests/lint will pass once dependency adobe/da-tools#2 is merged and updated here.

@bosschaert
Copy link
Contributor

bosschaert commented Feb 26, 2026

@hannessolo I don't see any unit tests for the new code in shareddoc.js? Just changes to make the existing tests work again.
It would be nice to get the new code covered as well.

@bosschaert bosschaert self-requested a review February 26, 2026 08:40
@hannessolo
Copy link
Contributor Author

@bosschaert you're right, I initially had some tests for the json->yjs conversion but those live in the shared library now, and I neglected the tests for the shared doc.

Added some now. I think the cases I covered should be enough but let me know if you find something that you think would also be good to cover.


function getDocType(docName) {
if (docName.endsWith('.json')) return 'json';
if (docName.endsWith('.html')) return 'html';
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this line does nothing because you'll return html regardless 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True... I somehow didn't like not covering the html case explicitly too (for readability) but now that I think about it again return 'html' also makes it pretty clear that the other case to json is html even without the if... hmm

Copy link
Contributor

@bosschaert bosschaert left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the tests!

@hannessolo hannessolo merged commit bd12cc2 into main Feb 26, 2026
5 checks passed
@hannessolo hannessolo deleted the i701 branch February 26, 2026 17:16
@tripodsan
Copy link
Contributor

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants