-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
gui: Show folder/device status on small screens #8643
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
gui: Show folder/device status on small screens #8643
Conversation
On larger screens, folder and device status is shown in a textual form directly next to folder and device titles. However, on small screens, only icons are currently shown, which may be ambiguous to new users, who cannot possibly know what a specific icon means (see [1]). Thus, on small screens only, display a new entry in folder/device info that contains the same textual information that is shown in the title on larger screens. [1] https://forum.syncthing.net/t/android-list-of-devices/19251 Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
For the record, the whole |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the strange ></span><span
dance, the duplication of string-key to icon / title mapping here seems excessive and bound to get out of sync when something is changed or added. As there is plenty of prior art, I'd suggest moving this to a common JS function instead.
🤖 beep boop I'm going to close this pull request as it has been idle for more than 90 days. This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved. To permanently exempt this PR from bot nagging, add the |
Can we keep this one open? I'm still going to work on it when I've managed to find enough time. |
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick remarks, didn't have time to look at the HTML changes thoroughly or do any testing yet.
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, especially how it makes the HTML much more readable. Some nit-picks and the important question whether we want to replace percentage display with just an icon on small screens?
- keep the th tags on the same line - always show syncing percentage - sort only alphabetically in JS Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state is a bit inconsistent, see the longer inline comment.
gui/default/index.html
Outdated
<span ng-switch="folderStatus(folder)"> | ||
<span ng-switch-when="syncing">({{syncPercentage(folder.id) | percent}}, {{model[folder.id].needBytes | binary}}B)</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move the ng-switch
to the <div>
above, to have one less level of nested <span>
s. That should help avoiding stray whitespace between them. The scanning
case then needs to be added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same applies to scanning, of course. IIUC the intended behavior is this:
- In the header: Scanning and syncing shows the percentage after the status text on large screens, and without the status text on small screens.
- In the newly added table row: Any state just shows the status text, we don't need to repeat the percentage there. The icon always reflects the state icon shown in the header, except for scanning and syncing, where the icon is only visible here.
One problem with this is that I think without any icons, it can be confusing whether the folder is currently scanning or syncing. Although the former displays only percentage, and the latter percentage and size, I don't think this is clear to those who aren't already familiar with the internals.
How about just displaying both the icon, percentage, and size together? The icon doesn't take that much space...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could display the icon in addition. But frankly I don't think anyone cares what process is currently running, just that it's chugging along and how much is finished. At least in the overview - if you want more detail, there will be the added row with complete status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now agree that displaying the icons in all states (on small screens) is better. Simply because when scanning, there is an extended period of time where no percentage is yet available, thus without an icon we'd have no status at all.
th on wrong line
Add the panel-status class to make sure there is a small gap when the device name is long and closes in on the status fields. Switch to a <div> element instead of <span> for consistency with the folder panels. That better shows the intention to have a right-floating block in relation to the device label text.
Get rid of an extra <span> wrapping, instead declaring the ng-switch attribute on the panel-status <div> itself. Scanning and syncing information is now always shown properly in xs and larger sizes. The icon on the panel header is always visible, because there are short spans where the percentage is not yet known / valid, which would otherwise lead to a completely blank space instead of the progress numbers. Fix a syntax error in folderStatusText() function (missing semicolon).
In the device panels, a separate remote-devices-panel class was introduced to wrap icons with a "display: inline-block" style. It is not needed for pure text nodes though, but on both icons. However, it only works when applied in an outer wrapping element around the icon <span>. Rename it to more generic inline-icon class and apply to both folder and device status panels where needed.
This matches up with folders again, and gets rid of the now no longer needed <span> level.
No longer used anywhere, as this is now done in HTML again.
Merge fixes by @acolomb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this was my last suggested state, I'm obviously OK with it. But let's give @tomasz1986 another chance to look and test.
Just tested and everything looks fine to me! |
@calmh OK to merge, or shall we wait until v1.27.1 is out? LGTM from my side. |
Merge away. |
* main: (69 commits) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) lib/scanner: Record inode change time for directories and symlinks (syncthing#9250) lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254) gui, man, authors: Update docs, translations, and contributors lib/fs: Better equality comparison in mtimefs cmd/stcrashreceiver: Add metrics for diskstore inventory cmd/stcrashreceiver: Minor cleanup, stricter file permissions cmd/stcrashreceiver: Add metrics for incoming reports ...
* main: (89 commits) build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288) lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) ...
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
gui: Show folder/device status on small screens
On larger screens, folder and device status is shown in a textual form
directly next to folder and device titles. However, on small screens,
only icons are currently shown, which may be ambiguous to new users, who
cannot possibly know what a specific icon means (see [1]). Thus, on
small screens only, display a new entry in folder/device info that
contains the same textual information that is shown in the title on
larger screens.
[1] https://forum.syncthing.net/t/android-list-of-devices/19251
Signed-off-by: Tomasz Wilczyński twilczynski@naver.com
Screenshots
Before:
After: