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.

Sign up for GitHub

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

Jump to bottom

feat(ui): Support displaying storage values in base-2 #118

Closed
wants to merge 7 commits into from
Closed

feat(ui): Support displaying storage values in base-2 #118

wants to merge 7 commits into from

Conversation

sowaret
Copy link

@sowaret sowaret commented Oct 14, 2022 •
edited
Loading

UI functionality in support of #2492 (equivalent PR for CLI: #2502).

  • Add bytesStringBase2 option to ui-preferences
  • Use new SizeDisplay component to pull in config
    • Separate old sizeWithFailures logic from JSX
  • Add Units dropdown to bottom Table
  • Move sizeDisplayName and new getErrorString methods to new utils/ui.ts file, for TypeScript support

image
image

Questions

  1. Curious if there's a more elegant way to create the Storage Unit label (not familiar with react-bootstrap)
  2. Replicated the tests from kopia/units_test.go but I'm not familiar with the << syntax in Go to make use of that byte count.

Notes

  • String concats in SourcesTable were not updated to use the SizeDisplay component.
  • Would like to test SizeDisplay; I don't have a lot of context on summ errors yet.

Thanks!

@sowaret sowaret mentioned this pull request Oct 14, 2022
Merged
@sowaret sowaret changed the title Support base-2 storage values feat(ui): Support displaying storage values in base-2 Oct 14, 2022
@sowaret
Move sizeWithFailures logic to SizeDisplay
a72909e 
A component is used now, so it makes sense to separate the error string
logic from the JSX output

- Add missing `setBytesStringBase2` field to `UIPreferencesContext`
- Update Provider `initialValue` to accept `Partial` (e.g. in test)
- Add tests; add missing base-2 cases in `ui.test.ts`
- Update the empty error string logic from `sizeWithFailures` to check
that all fields are empty:

`if (!summ || !summ.errors || !summ.numFailed)` to
`if (!summ?.errors?.length && !summ?.numFailed)`
@jkowalski
Copy link
Contributor

jkowalski commented Oct 15, 2022 •
edited
Loading

nit: can we collapse the label + value into a dropdown with 2 options:

  • Units: Decimal
  • Units: Binary

The rest LGTM modulo linter errors.

@sowaret
Copy link
Author

sowaret commented Oct 17, 2022 •
edited
Loading

In testing, I'm running into troubles with act() and the DropdownMenu state change; I'm not very familiar with the Context API but I imagine it's originating from there. Tests pass, but there's a fix for that I'm missing there.

I've also put together a render solution that plays well with UIPreferenceProvider given that it GETs the values on load (for testing purposes this is unnecessary). Doesn't seem very elegant having to async/await every test render. Two things I imagine would simplify that would be removing the mix of TypeScript and JavaScript, and refactoring that provider a bit (it PUTs twice on load even when preferences aren't being saved, for example, so some cleanup down the line wouldn't hurt).

Feel free to call out if anything looks off. Thank you.

@lupusA
Copy link
Collaborator

lupusA commented Jul 8, 2023

This PR is obselet with #150. The preferences tab now allows changing the byte representation.

Cheers,

@lupusA lupusA mentioned this pull request Jul 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sowaret @jkowalski @lupusA