search

ubc-carnap-team / Rudolf

Truth Tree Widget for Carnap
2 stars 3 forks source link

JSONView collapse on click #61

Closed mbecker20 closed 4 years ago

mbecker20 commented 4 years ago

solves issue #57

mbecker20 commented 4 years ago

the last three commits (1, 2, 3) solve the first part of issue 57 with a button, as you suggested

orbit-stabilizer commented 4 years ago

Looks good to me - especially visually! Let's see what @McTano has to say.

mbecker20 commented 4 years ago

As for the second part of 57, I got close to the required functionality by using a useState initialized to JSON.stringify(...). The value of the textarea becomes the return value of the useState. Then I think onChange just needs to dispatch the new value to the store. But now to update the value in JSONView, we would need to use the setValue func returned by the useState any time the state is updated outside of JSONView. Does this sound about right?

McTano commented 4 years ago

As for the second part of 57, I got close to the required functionality by using a useState initialized to JSON.stringify(...). The value of the textarea becomes the return value of the useState. Then I think onChange just needs to dispatch the new value to the store. But now to update the value in JSONView, we would need to use the setValue func returned by the useState any time the state is updated outside of JSONView. Does this sound about right?

Yes, but it's a bit simpler then that. Adding another useState is unnecessary. The value of the text area is computed from the component's props, so there's no need to manually update it locally. If the store gets updated, the props will change, and the component will automatically update through React magic. The second part of your answer is right, though. The way to do this would be to add an action to the rudolf store which takes JSON representing the app state and updates the store, then dispatch it in the onChange handler.

However, after thinking about it, I no longer think this is a good idea, since the user would have to update basically the whole app state at once to avoid invalid creating an invalid state in between (and potentially crashing the component). If we want to be able to restore the component from saved state, it would be better to do it a different way.

McTano commented 4 years ago

The current changes look good. Don't worry about the update feature. You can go ahead and merge it.

McTano commented 4 years ago

I tried JSONView.styles.ts and it broke, so instead of messing with the file extension I went with JSONView_styles.ts, which works.

McTano commented 4 years ago

I'm gonna go ahead and merge this so I can resolve my checker's conflicts with master and get the sort of working checker deployed.