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

New tool JSON>Table/CSV/Excel #1003

Merged
merged 5 commits into from
Dec 18, 2023
Merged

Conversation

sakana280
Copy link
Contributor

@sakana280 sakana280 commented Dec 16, 2023

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Requests for JSON<>CSV conversion in
#200
#671
#708

What is the new behavior?

  • Converts a JSON array of objects to a grid view that can be copied to clipboard as CSV/TSV/FSV.
  • Flattens nested JSON objects and ignores any nested arrays eg {"a":{"b":1,"c":{"d":2},"e":[3,4]}} becomes {"a_b":1,"a_c_d":2}
  • Smart detection triggered by any JSON. Should this be smarter and only detect on JSON arrays?
  • Any conversion errors (eg not valid JSON, not a JSON array) show as an error message in a single-cell datagrid, and cannot copy to clipboard (button is disabled)
  • Includes GUI and CLI tools.
  • Would resolve JSON to Table #708, and half of JSON < > CSV (or Excel) #200

Other information

image

Possible future ideas:

  • Convert TSV/CSV to JSON:
    • Add a 'paste' button to the datagrid to paste TSV/CSV/CSV-French (auto detect or use the setting?) then convert to json
    • Leave datagrid on right hand side but remove 'input'/'output' labels since the datagrid can be input via the paste mechanism
    • This removes complexity of error handling with a toggle between grid view and text view of the TSV/CSV.
    • Would resolve CSV / XML / Excel to JSON #671 and JSON < > CSV (or Excel) #200
  • Respond to compact mode, showing as vertical layout.

Quality check

Before creating this PR:

  • Did you follow the code style guideline as described in CONTRIBUTING.md
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS (DevToys 2.0)
    • Linux (DevToys 2.0)

I couldn't build Release, but also couldn't build Release for the base dev/2.0 branch. Error message is:
Could not write to output file 'C:\Programming\DevToys.v2\obj\AnyCPU\DevToys.Business\Release\net7.0\generated\CommunityToolkit.Mvvm.SourceGenerators\CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator\DevToys.Business.ViewModels.ToolPageViewModel.ToggleSelectedMenuItemFavorite.g.cs' -- 'Could not find a part of the path 'C:\Programming\DevToys.v2\obj\AnyCPU\DevToys.Business\Release\net7.0\generated\CommunityToolkit.Mvvm.SourceGenerators\CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator\DevToys.Business.ViewModels.ToolPageViewModel.ToggleSelectedMenuItemFavorite.g.cs'.' DevToys.Business (net7.0) C:\Programming\DevToys.v2\src\app\dev\DevToys.Business\CSC

@veler
Copy link
Collaborator

veler commented Dec 17, 2023

A thought on the UI:

This is currently NOT achievable with the API we have, but how about: instead of having a Setting where we chose between CSV, TSV, FSV, we have a drop down button in the command bar to copy as CSV, TSV, FSV, along with "Save as..." (which would suggest CSV...etc).

Something similar to what there is in the text box when the UI gets too small:
image

Except that user could choose between CSV, TSV, FSV.

What do you think @sakana280 ?

Let's no block your PR on this idea, since there's no API to achieve this. It's just something for me to think about for a future PR.

Copy link
Collaborator

@veler veler left a comment

Choose a reason for hiding this comment

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

Hey @sakana280, it looks GREAT in overall :D Thank you so much for this!
Just a few suggestions and we should be good to go.

Improved naming for CLI tool and UI elements.
Don't overflow large JSON lists.
@sakana280
Copy link
Contributor Author

Cheers heaps for the review. I've addressed all the comments (I think?) except with a query about retaining "Excel" in the search terms only.

@veler veler linked an issue Dec 17, 2023 that may be closed by this pull request
@veler veler added the devtoys-v2.0 DevToys v2.0 label Dec 17, 2023
@veler
Copy link
Collaborator

veler commented Dec 17, 2023

Hi @sakana280 ,

Thanks for addressing my feedback so quickly. It remains 1 tiny fix to do and we should be all good to go 😍

Cheers !

Copy link
Collaborator

@veler veler left a comment

Choose a reason for hiding this comment

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

Hello,
It looks great now, thank you so much for your contribution! 😁

@veler veler merged commit bea8711 into DevToys-app:dev/2.0 Dec 18, 2023
@sakana280
Copy link
Contributor Author

Many thanks @veler for helping me navigate the codebase and your speedy review of this PR :)

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

Successfully merging this pull request may close these issues.

JSON < > CSV (or Excel)
2 participants