-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
Thanks for working on this! It looks good, except that functions should be moved outside, so they are not recreated every time the component is updated. However, we could remove them at all and just pass Agree that it should be opt-in as it could affect the performance on large amount of keys. Probably better to add that when we move to the new UI as there's a toggle button (see record button here) and tooltip. The button state should be persisted in reducers. |
@zalmoxisus thanks for the review! I like the idea of simply passing a boolean |
Thanks for the help. I think better to wait for that pr mentioned above and if we go with reduxjs/redux-devtools#412 to move your pr there, not to lose the contribution history. |
OK, I'll keep an eye on alexkuz/react-json-tree#108 and the monorepo work, with the intent to make a separate PR in the |
Hey again! I noticed that alexkuz/react-json-tree#108 was merged a few hours ago-- thanks for diligently moving this forward! Is it feasible for me to create a PR against the |
Hey! Thanks for following and helping with it! I was busy with As per this issue, adding a button like I suggest before, wouldn't be so useful, as it's not something users will switch frequently. I think we need this in settings, but first will need to implement settings specific to monitors. Or we could add it also for So the plan would be to add it first to the props of the monitor, then adding the ability for changing this prop from the extension's settings page (another PR later). I'll move the package today and you can start a PR when you have time. For the second part better to wait for the new UI, where settings will be a tab in the app, instead of using browser option page and syncing from there. Hopefully we'll have everything in one place soon, and it will be much easier to contribute. Feel free to assist with the PRs of transitioning the packages there. Thanks! |
@milotoor I merged the monitors and created an issue there: reduxjs/redux-devtools#433. |
@milotoor @zalmoxisus isthere any update on this issue? Are we expecting this to be part of next release? |
@akash-pal please refer to the issue @zalmoxisus mentioned before. |
This will sort the JSON tree keys in alphabetical order, rather than with the implementation-dependent ordering determined by the browser engine's
getOwnPropertyNames
function. In large JSON structures, it can be challenging to locate a particular nested node of interest among the multitude of other keys. Adding this sorting will dramatically simplify locating specific state attributes. The request comes from zalmoxisus/redux-devtools-extension#416.Note that the
localeCompare
function is supported in all major browsers per MDN. It is case-sensitive, soabc
will come beforeAbc
.If it's preferable that this be opt-in so as not to upset existing applications, I'm more than happy to make changes as advised. Thanks!