Skip to content

Create a JSON diff tool#5

Open
xJuliMendez wants to merge 4 commits intokmilogp:mainfrom
xJuliMendez:feature/json-diff
Open

Create a JSON diff tool#5
xJuliMendez wants to merge 4 commits intokmilogp:mainfrom
xJuliMendez:feature/json-diff

Conversation

@xJuliMendez
Copy link

No description provided.

Copy link
Owner

@kmilogp kmilogp Oct 19, 2024

Choose a reason for hiding this comment

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

I think we could remove the base editors and use only the diff editor for this feature. A few reasons:

  • The input and output editor occupy most of the screen, which makes the scroll unavoidable when the diff appears
  • A second iteration for checking a diff implies having to go to the top of the screen to generate a diff and then go bottom again to see the result
  • The diff viewer per se has all the desired features, so we could just use it and even have real time feedback for the user

I would suggest

  • Removing the input and output editor
  • Adding the editor toolbar for copying the content to the clipboard
  • Removing the diff button, it's no necessary for the feature, since probably we won't need error handling

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Will be adding it. In the end the diff editor was more powerfull than I expected. All your feedback was in a way the correct usage of the editor.

Copy link
Owner

@kmilogp kmilogp left a comment

Choose a reason for hiding this comment

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

  • Removing the input and output editor
  • Adding the editor toolbar for copying the content to the clipboard
  • Removing the diff button, it's no necessary for the feature, since probably we won't need error handling

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants