Fix #365: Render a JSON diff for history entries. #380
Conversation
n1k0
commented
Feb 1, 2017
•
n1k0
commented
|
This is wrong, it computes diffs with the immediate previous entry in the list, we need to find the appropriate resource instead. Will rework. |
|
Yeah, unfortunately because of pagination I guess that the easiest to achieve this is to issue a request when the details panel is opened... With something like |
|
Would it be hard to eventually have the previous version of the resource attached to the change event on the server? |
It's more a matter of duplication than complexity... I'd rather be honest, unless someone comes with a nice plan and implements it, we probably won't spend time to change that :| |
So that's delegating the tedious work to client implementers, that's fine by me but still I feel this would be nice to have an option or smthg. No big deal, I'll do the atomic request thing to compute the diff. |
Yeah I know... not that the server stuff was not tedious to implement huh ;) We discarded the option to duplicate both previous and current in every entry. If we would have done something on the server side, it would have been to store the diffs only. But then the clients would have to apply every diff on top of each other to obtain previous versions. So we found that compromise... |
|
No problem, at least that keeps me busy ;) |
|
@leplatrem made the changes, r=? |
src/components/HistoryTable.js
Outdated
| if (this.state.previous) { | ||
| return this.setState({open: true}); | ||
| } | ||
| fetchPreviousVersion(bid, entry) |
There was a problem hiding this comment.
I think we can avoid this when action == create (no history) or delete (contains deleted object already)
src/components/HistoryTable.js
Outdated
| ? <Diff source={entry.target} target={previous.target} /> | ||
| : error | ||
| ? <p className="alert alert-danger">{error}</p> | ||
| : <p className="alert alert-info">This resource has no previous versions.</p>} |
There was a problem hiding this comment.
When clicking on the details of create, we may want to keep <pre>{JSON.stringify(entry, null, 2)}</pre> no?
src/components/HistoryTable.js
Outdated
| } | ||
| fetchPreviousVersion(bid, entry) | ||
| .then((previous) => this.setState({open: true, previous, error: null})) | ||
| .catch((error) => this.setState({open: true, previous: null, error})); |
There was a problem hiding this comment.
Maybe we can add a comment here why it is acceptable not to use redux here :)
|
@leplatrem final r=? |
|
I really like it :) GG! |
New features * Fix #377, #378: Allow dropping edited resource properties. (#379) * Fix #365: Render a JSON diff for history entries. (#380) * Fix #376: Denote readonly buckets & collections in the sidebar. (#382) * Fix #384: Live-searchable/filterable sidebar entries. (#385) * Hide auth method selector when a single one is configured. Bugfixes * Do not store passwords. Fixes #364 (#386)
