payload icon indicating copy to clipboard operation
payload copied to clipboard

feat: allows fields to be collapsed in the version view diff

Open franciscolourenco opened this issue 1 year ago • 11 comments

Description

Allows some fields to be collapsed in the version diff view. The fields that can be collapsed are the ones which can also be collapsed in the edit view, or that are logically grouped. Currently collapsible, group, array (and their rows), blocks (and their items), and tabs.

Future improvements

  • Currently, collapsibles inside other collapsibles will forget their state if their parent is collapsed and then uncollapsed. We could improve this by setting display: none on the content, instead of not rendering it. Or storing the state somewhere.
  • Use custom row labels to render the array row labels
  • Render the block title in the block item collapsible label

Screenshots

SCR-20240905-jiko SCR-20240905-jiin image

https://github.com/user-attachments/assets/956d173f-7bce-4bb1-acb0-2f2eb7728ae8


  • [x] I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • [x] New feature (non-breaking change which adds functionality)

Checklist:

  • [x] I have added tests that prove my fix is effective or that my feature works
  • [ ] Existing test suite passes locally with my changes
  • [ ] ~I have made corresponding changes to the documentation~

franciscolourenco avatar Sep 04 '24 00:09 franciscolourenco

There are 2 additional things which block authors today from using the diff:

  1. The fact that we see all fields and not only changed fields
  2. The way RichText data is presented as Json and not Text I will try to work out those 2 and will update on discord if I manage

tsemachh avatar Sep 04 '24 10:09 tsemachh

@tsemachh I actually halfway into implementing 2. (rich text rendering in the diff view). Have you started on it already?

Also you might be interested in this one too. It was implemented in v2 , but shouldn't be too hard to port to v3.

franciscolourenco avatar Sep 04 '24 10:09 franciscolourenco

Regarding the design, I think the one thing missing here is some sort of indication that a collapsed group has changes or not. perhaps highlighting the group when collapsed to show it has diffed children would be an improvement.

tylandavis avatar Sep 05 '24 13:09 tylandavis

@tylandavis what about something like this?

https://github.com/user-attachments/assets/16faa8fc-154c-42fc-b877-94d2422006e6

franciscolourenco avatar Sep 05 '24 15:09 franciscolourenco

@tsemachh what do you think of the current state. Did you have something else in mind?

franciscolourenco avatar Sep 30 '24 12:09 franciscolourenco

@franciscolourenco It's taking forever to review the PR , I think we need to move forward with it , What I see in the video is a good progress. I will be very happy to see the rich text diff which is the main issue left (Not mentioning my PR of show modified fields which no one looked at it for few weeks)

tsemachh avatar Sep 30 '24 13:09 tsemachh

Thanks for the input @tsemachh! I actually meant to tag @tylandavis, sorry.

franciscolourenco avatar Sep 30 '24 13:09 franciscolourenco

Hey there @franciscolourenco, my apologies for the delay in my response and thanks for your patience. I have just two notes on the changes here:

  1. I think the indicator should be persistent whether collapsed or not, so that it isn't blinking in and out when a user toggles. This can also make it easier to find collapsible fields if browsing through a larger document.
  2. I think instead of a + - icon, if we could use a <Pill> component that includes the number of fields with changes i.e. 3 fields changed that would be useful as well.

What do you think?

tylandavis avatar Oct 11 '24 18:10 tylandavis

@tylandavis makes sense. Just to confirm,

  1. For the calculation of the number, should we use only direct children or take into account all fields even if nested deep?
  2. If we go with the nested approach, if a collapsible/array/blocks/group field has other fields with changes inside, would you count that the collapsible/array/blocks/group itself as a changed field, or only consider it's children?

franciscolourenco avatar Oct 16 '24 15:10 franciscolourenco

This PR is stale due to lack of activity.

To keep the PR open, please indicate that it is still relevant in a comment below.

github-actions[bot] avatar Dec 15 '24 05:12 github-actions[bot]

Still relevant, will fix merge conflicts soon.

franciscolourenco avatar Dec 15 '24 05:12 franciscolourenco

@tylandavis I've added a changed-fields-count pill as you suggested and brought back the vertical gutters because without them it was hard to understand the parents of the nested fields.

The changed-fields-count pill counts fields recursively and not only the immediate children because that looked quite confusing.

Would you mind having another look, and letting me know if there are any other visual changes necessary?

Thanks!

franciscolourenco avatar Jan 14 '25 09:01 franciscolourenco

🚀 This is included in version v3.20.0

github-actions[bot] avatar Jan 29 '25 15:01 github-actions[bot]