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

luci-app-opkg: show disk space consistent with overview #6897

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

stangri
Copy link
Member

@stangri stangri commented Feb 8, 2024

I'm attaching screenshot of the change, which is consistent how space is displayed in Status->Overview
image

@stangri stangri force-pushed the master-luci-app-opkg branch 2 times, most recently from e8878f5 to de3b887 Compare February 8, 2024 04:24
@jow-
Copy link
Contributor

jow- commented Feb 8, 2024

Please submit the change without the entire file reformatted, otherwise it is impossible to see actual changes.

@stangri stangri force-pushed the master-luci-app-opkg branch 2 times, most recently from 2ad8ac1 to 6dc4c91 Compare February 8, 2024 15:12
@stangri
Copy link
Member Author

stangri commented Feb 8, 2024

@jow- sorry about that, turns out it was a different editor (subl) I've used to prepare PR which was formatting the file differently. I've noticed there were two occurrences of 4 spaces used instead of the tab in the file, I've replaced those. Let me know if you want it reflected in the commit message.

@stangri stangri force-pushed the master-luci-app-opkg branch from 6dc4c91 to 3fb99fd Compare February 8, 2024 15:17
@stangri stangri force-pushed the master-luci-app-opkg branch from 3fb99fd to 60ae987 Compare February 9, 2024 16:43
@stangri stangri requested review from jow- and hnyman February 9, 2024 18:43
@hnyman
Copy link
Contributor

hnyman commented Feb 9, 2024

I did not run test it, but looks nice.

However, I am thinking if the percentage number is actually useful at all, as the remaining free space in MiB is the most relevant info at the package installation page (to determine how much can still be installed).

I wonder if it might be more useful to drop the percentage number from text (but keep the graph bar), and change the text to be something like "7.2 MiB used of 16 MiB, 8.8 MiB still free"

@stangri
Copy link
Member Author

stangri commented Feb 9, 2024

However, I am thinking if the percentage number is actually useful at all, as the remaining free space in MiB is the most relevant info at the package installation page (to determine how much can still be installed).

I wonder if it might be more useful to drop the percentage number from text (but keep the graph bar), and change the text to be something like "7.2 MiB used of 16 MiB, 8.8 MiB still free"

This matches the suggestion on the forum as well, to display the available space in MiB better. Let me sleep on it/play with various options and I'll update PR within 72-96 hours.

* Show Disk space graph consistent with the Status->Overview page
* Brought up in https://forum.openwrt.org/t/software-space-is-going-to-full/187112
* Leading whitespaces reformatted by vscode
* Localizable disk space progress bar title
* Kudos to https://forum.openwrt.org/u/psherman for coming up with final design

Signed-off-by: Stan Grishin <[email protected]>
@stangri stangri force-pushed the master-luci-app-opkg branch from 60ae987 to c80ca79 Compare February 10, 2024 02:25
@stangri
Copy link
Member Author

stangri commented Feb 10, 2024

@jow- @hnyman please re-review.

@stangri
Copy link
Member Author

stangri commented Feb 12, 2024

It's a small enough change I feel I can merge it with reviews from @hnyman and @systemcrash -- thank you gentlemen for quick responses.

@jow-, @McGiverGim thank you for the feedback helping improve this PR!

@stangri stangri merged commit eb40b0b into openwrt:master Feb 12, 2024
5 checks passed
systemcrash pushed a commit that referenced this pull request Feb 14, 2024
luci-app-opkg: show disk space consistent with overview
(cherry picked from commit eb40b0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants