Skip to content
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

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

zzacharo
Copy link
Member

@zzacharo zzacharo commented Jan 18, 2025

@zzacharo zzacharo force-pushed the compare-record-revisions branch 2 times, most recently from 12a8ee1 to fc10fd5 Compare January 23, 2025 16:16
@zzacharo zzacharo force-pushed the compare-record-revisions branch 2 times, most recently from f7adb3b to 6c697c2 Compare January 23, 2025 17:05
@zzacharo zzacharo self-assigned this Jan 24, 2025
@zzacharo zzacharo force-pushed the compare-record-revisions branch 2 times, most recently from 30e7244 to dd38008 Compare January 24, 2025 13:48
@zzacharo zzacharo removed their assignment Jan 24, 2025
@kpsherva
Copy link
Contributor

UI, general comment: what about putting the diff window inside a fluid container to take the full modal width instead of segment
you could use a divider to separate (is separation needed?)
it looks a bit strange with the white space, unused on the right hand side

@zzacharo zzacharo force-pushed the compare-record-revisions branch 2 times, most recently from 50c8ab8 to 528aa9c Compare January 24, 2025 17:20
Copy link
Contributor

@jrcastro2 jrcastro2 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 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: {
Copy link
Contributor

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

Copy link
Member Author

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 });
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover

@zzacharo zzacharo force-pushed the compare-record-revisions branch from 528aa9c to 56f79da Compare January 27, 2025 10:52
@zzacharo zzacharo force-pushed the compare-record-revisions branch from 56f79da to 6760f52 Compare January 27, 2025 11:04
@zzacharo zzacharo merged commit 394e605 into inveniosoftware:master Jan 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To release 🤖
Development

Successfully merging this pull request may close these issues.

administration: add support for comparing revisions of one record's version
3 participants