Skip to content

html diff in revision view - #139

Merged
ssddanbrown merged 2 commits into
BookStackApp:masterfrom
younes0:diff
Sep 29, 2016
Merged

ssddanbrown merged 2 commits into
BookStackApp:masterfrom
younes0:diff

Conversation

@younes0

@younes0 younes0 commented Jul 7, 2016

Copy link
Copy Markdown

Result:
image

@ssddanbrown

Copy link
Copy Markdown
Member

@younes0 Thanks for this. Unfortunately the library used, icap/html-diff, is licensed under GPL2+ which, as far as I'm aware, is not compatible with the MIT licence used in BookStack :(

@younes0

younes0 commented Jul 10, 2016

Copy link
Copy Markdown
Author

@ssddanbrown Ooops sorry for that. I'll push with this lib instead https://github.com/gathercontent/htmldiff

Note that this one will require the php-tidy ext

replace gpl lib with mit lib
@younes0

younes0 commented Jul 10, 2016

Copy link
Copy Markdown
Author

done!

@ssddanbrown

Copy link
Copy Markdown
Member

Hey @younes0,
Thanks again for this feature. Just want to explain why I haven't merged this in yet. It's just the additional php-tidy extension requirement that's stopping me merging at the moment. I didn't want to introduce a breaking update right away so I'm seeing if there's some other things that can be added to BookStack that will require potential breaking direct update compatibility.

@younes0

younes0 commented Sep 13, 2016

Copy link
Copy Markdown
Author

Hello @ssddanbrown
As far as I remember, I couldn't find a MIT licenced package that didn't require php-tidy

@ssddanbrown
ssddanbrown merged commit c279c6e into BookStackApp:master Sep 29, 2016
@ssddanbrown

ssddanbrown commented Sep 29, 2016

Copy link
Copy Markdown
Member

Yeah, I also had a look around but could not find a better library. Also tried making the diff code myself but failed pretty miserably.

There's going to be some compatibility changes/breakages in the next BookStack release anyway so now is a good time to sneak in the php-tidy requirement. Should be handy to have tidy in other areas of BookStack anyway.

Thanks again for your work on this.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants