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

Create a JSON diff tool #5

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xJuliMendez
Copy link

No description provided.

Copy link
Owner

@kmilogp8496 kmilogp8496 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

@kmilogp8496 kmilogp8496 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