Skip to content
This repository has been archived by the owner on Mar 27, 2019. It is now read-only.

52 json diff patch #84

Merged
merged 3 commits into from
Jun 14, 2017
Merged

52 json diff patch #84

merged 3 commits into from
Jun 14, 2017

Conversation

Lucretius
Copy link
Collaborator

Fairly basic implementation of the json diff patch request here: #52

This will show up for all JSON editors used throughout the project. By default the differ is turned on, and this refreshes every time a new editor instance is generated. Not sure how we want to handle specifics (do we want localStorage keys for each editor? aka one for the secret JSON, and a different one for, say, the JSON wrapper editor?)

I'm open to suggestions. See the corresponding issue for an example of what the PR change looks like in Vault.

@msessa
Copy link
Collaborator

msessa commented Mar 31, 2017

Surprisingly contained change :) I feared this would take way more code.

In terms of UX, I reckon we could wrap the JSON editor into a Stepper component to enforce a Edit JSON->Review Changes workflow.

@Lucretius
Copy link
Collaborator Author

@msessa

Yeah, throwing it into the JSON editor let me keep it real short.

As far as the UX goes, this is a bit of an age old question - to stutter step or not? In almost all circumstances I avoid enforcing a paradigm that would require a second step each time someone wants to save their changes, and simply allow them to view a diff if desired (or, alternatively, have some optional confirmation dialog pop up - the key being optional).

I know that the original issue called for such a UX change, but I'd argue we should leave it as is. It acts now much like a file diff here on Github - something we can view if we want, but won't add an extra step when it comes time to take action.

@msessa
Copy link
Collaborator

msessa commented Mar 31, 2017

How about allowing a user to disable the review step with a checkbox in the preferences?

@Lucretius
Copy link
Collaborator Author

Just for the secrets? This current JSON differ shows up anywhere there is a JSON Editor in use and in several contexts the save functionality doesn't make sense. I'd still opt for no review step regardless.

Let's think about the purpose of the review step. It's to get the user to stop and think about if the changes they're about to make are the ones they want to make. We would essentially ask "are you sure?" A user in the app who pressed the button in the first place is sure - they made the changes. The option to preview their changes will exist, which they can view at any time if they want to review prior to saving. I see no need to bake the paradigm into the application itself.

Code wise it also means we will need to add save handlers, which are outside of their respective JSON editors - which will bloat the scope of this PR.

I'd ask @alexunwin and @djenriquez for their input but I'd much rather leave out stutter steps.

@djenriquez
Copy link
Owner

Nice work. It makes sense to allow the option wherever JSON modifying exists. This is fine.

I think it would be better experience for the user if the diff was an option that was enabled rather than being on all the time....speaking of which, how do I enable it to test?

@djenriquez
Copy link
Owner

screen shot 2017-04-02 at 10 40 42 pm

Don't see a diff anywhere nor any options to turn it on?

@Lucretius
Copy link
Collaborator Author

You sure you on the right branch? Check the code in the JSONEditor JSX file. It should be present everywhere those editors are. Check the console to see if you forgot to npm install anything

@djenriquez
Copy link
Owner

Yup

$ git branch
* 52-json-diff-patch
  master
$ git pull
Already up-to-date.

@djenriquez
Copy link
Owner

djenriquez commented Apr 4, 2017

Oh, its at the bottom below my scrollbar....hmmmm

@Lucretius anyway you can fit the diff to shrink the modal such that users don't have to scroll down to see the diff?

@djenriquez
Copy link
Owner

@Lucretius Can you adjust the sizing so that users don't have to scroll down to see the diff?

@djenriquez djenriquez merged commit e54ecb3 into master Jun 14, 2017
@djenriquez djenriquez deleted the 52-json-diff-patch branch June 14, 2017 06:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants