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

Consistent JSON Output Format #188

Merged
merged 8 commits into from
Jul 14, 2020

Conversation

jlgoff3
Copy link
Contributor

@jlgoff3 jlgoff3 commented Jun 1, 2020

Fixes #94

Problem

Currently usage of JSON requires Raw JSON in -> TipTap v-model -> Serialized JSON out. The type of the content is object up until the first TipTap edit is made, at which point it becomes a string. My team is using TipTap-Vuetify in our ERP project, and we've previously had to write code to get around this that checks to see whether it is a string or object on save.

Purpose

This PR is intended to make the behavior of output-format="json" more consistent, namely:
Raw JSON in -> TipTap v-model -> Raw JSON out.

Current behavior of TipTap-Vuetify with output-format="json" is inconsistent. V-model input expects a raw JSON object, but causes [Vue warn] type error and changes output to a serialized JSON object.

Expected behavior is that when output-format="json", TipTap-Vuetify accepts a raw JSON object as v-model input with proper type checking and outputs raw JSON object. This PR makes that behavior consistent.

Replication

Replication - Replication has some issues due to codesandbox's platform limitations. Instructions in the sandbox demonstrate the inconsistent behavior.

Any comments, questions, or recommendations are appreciated. Thank you for your work maintaining this project! It's been very useful in our own project.

jlgoff3 added 2 commits June 1, 2020 10:20
removed serializing of JSON in output, added Object type to v-model prop
@jlgoff3 jlgoff3 marked this pull request as ready for review June 1, 2020 15:08

@Prop({ type: String, default: '' })
readonly [PROPS.VALUE]: string
@Prop({ type: [String, Object], default: "" })
Copy link
Owner

Choose a reason for hiding this comment

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

This is first key moment.

} else {
output = JSON.stringify(info.getJSON())
output = info.getJSON();
Copy link
Owner

Choose a reason for hiding this comment

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

This is second key moment.

@iliyaZelenko
Copy link
Owner

Hello!

First of all, thanks for a very detailed description of the problem and its solution. It was very informative and I immediately understood the essence of the problem.

It seems like a funny situation arose when your IDE changed the style of the code that does not correspond to my project, which caused ESLint errors on CI.

I see two ways out of the situation:

  1. I can take the key points of your changes (I indicated them in the code, maybe I missed something) and make a new Pull Request myself.
  2. You will remove unnecessary changes from this PR and I will accept it.

What will we choose?

Thank you for contributing to the project.

@jlgoff3
Copy link
Contributor Author

jlgoff3 commented Jun 1, 2020

Oops! I had Prettier enabled on save, and it must have changed the styling. I ran yarn lint-fix and pushed, so I believe it should be working now. Thanks for the quick response!

@iliyaZelenko
Copy link
Owner

Now much better, but still removed a lot of line breaks that I would like to see + CSS styles affected.

I suggest copying the source file to return to its original state and add your changes to it. Only a couple of lines will need to be changed again.

@jlgoff3
Copy link
Contributor Author

jlgoff3 commented Jun 1, 2020

Hmm, that's what I did last time, but I tried it again, and it seems to have worked. Hope that does it for you. Thanks for working through it with me.

@jlgoff3
Copy link
Contributor Author

jlgoff3 commented Jun 26, 2020

Hi, just bumping this to check and see if it's in an acceptable state to be merged. Thanks!

@iliyaZelenko iliyaZelenko merged commit 80bade0 into iliyaZelenko:master Jul 14, 2020
@iliyaZelenko
Copy link
Owner

LGTM, TY!

@jlgoff3 jlgoff3 deleted the json-output-fix branch July 14, 2020 12:27
iliyaZelenko pushed a commit that referenced this pull request Jul 19, 2020
# [2.24.0](v2.23.0...v2.24.0) (2020-07-19)

### Bug Fixes

* Consistent JSON Output Format ([#188](#188)) ([80bade0](80bade0)), closes [#94](#94)
* **i18n:** improve fr translation ([#207](#207)) ([d964357](d964357))

### Features

* **i18n:** el translation ([#195](#195)) ([6e17964](6e17964))
@iliyaZelenko
Copy link
Owner

🎉 This PR is included in version 2.24.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v-model not working with output-format="json"
2 participants