Skip to content
This repository has been archived by the owner on Jun 5, 2020. It is now read-only.

Sort JSON tree keys alphabetically #80

Closed
wants to merge 1 commit into from
Closed

Sort JSON tree keys alphabetically #80

wants to merge 1 commit into from

Conversation

milotoor
Copy link

@milotoor milotoor commented Nov 16, 2018

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, so abc will come before Abc.

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!

@zalmoxisus
Copy link
Collaborator

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 true when we get alexkuz/react-json-tree#108 merged, which should be faster.

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.

@milotoor
Copy link
Author

@zalmoxisus thanks for the review! I like the idea of simply passing a boolean sortObjectKeys to sort alphabetically by default. I would be happy to work on a PR to add a button to the new UI, if that's productive?

@zalmoxisus
Copy link
Collaborator

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.

@milotoor
Copy link
Author

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 redux-devtools repo when that gets sorted out. If you need help with the monorepo activity I'm happy to lend a hand, just ping me if I can be useful!

@milotoor milotoor closed this Nov 19, 2018
@milotoor
Copy link
Author

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 redux-devtools repo now for this feature? I'm not sure how you are approaching the monorepo work, or if the redux-devtools-inspector has been migrated yet. If it hasn't, I'm happy to assist transitioning the package, though again I'm not sure what the approach is :)

@zalmoxisus
Copy link
Collaborator

zalmoxisus commented Dec 21, 2018

Hey! Thanks for following and helping with it! I was busy with redux-devtools-extension releases last weeks. Now back to the monorepo:)
I've just merged react-json-tree. Will work now on redux-devtools-inspector, there are some changes in my fork which will be merged as well. Should be done today.

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 redux-devtools-log-monitor and use in general settings. That repo will be merged as well.

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!

@zalmoxisus
Copy link
Collaborator

@milotoor I merged the monitors and created an issue there: reduxjs/redux-devtools#433.
There are some other issues there related for the migration if you'll find them interesting.

@akash-pal
Copy link

@milotoor @zalmoxisus isthere any update on this issue? Are we expecting this to be part of next release?

@alexkuz
Copy link
Owner

alexkuz commented Jun 5, 2020

@akash-pal please refer to the issue @zalmoxisus mentioned before.
I'm archiving this repository now.

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.

None yet

4 participants