-
Notifications
You must be signed in to change notification settings - Fork 150
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
administration: compare record revisions #2962
administration: compare record revisions #2962
Conversation
12a8ee1
to
fc10fd5
Compare
...p_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/administration/records/CompareRevisions.js
Outdated
Show resolved
Hide resolved
...p_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/administration/records/CompareRevisions.js
Outdated
Show resolved
Hide resolved
...p_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/administration/records/CompareRevisions.js
Show resolved
Hide resolved
f7adb3b
to
6c697c2
Compare
30e7244
to
dd38008
Compare
...p_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/administration/records/CompareRevisions.js
Outdated
Show resolved
Hide resolved
...p_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/administration/records/CompareRevisions.js
Show resolved
Hide resolved
UI, general comment: what about putting the diff window inside a fluid container to take the full modal width instead of segment |
50c8ab8
to
528aa9c
Compare
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.
LGTM! Thanks a lot!
Seems there are some very minor missing destructuring props and unused props warnings.
this.setState({ | ||
modalOpen: true, | ||
modalHeader: i18next.t("Compare revisions"), | ||
modalProps: { |
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.
Is this modalProps
used? or just a leftover
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 updated the implementation, it was not used indeed :)
const { error, loading, allRevisions, srcRevision, targetRevision, diff } = | ||
this.state; | ||
|
||
console.log({ diff }); |
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.
leftover
528aa9c
to
56f79da
Compare
56f79da
to
6760f52
Compare
requires inveniosoftware/invenio-rdm-records#1929
requires inveniosoftware/invenio-administration#233
closes #2968