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

Show sync indicator in account header #2560

Merged
merged 6 commits into from
Apr 7, 2024

Conversation

psybers
Copy link
Contributor

@psybers psybers commented Apr 6, 2024

Currently, sync indicators are only shown for desktop in the accounts list on the left sidebar. If you select a single account and ask to sync it, the indicator on the left is not visible for that account (since it is selected):

image image

Thus you will not know if the sync failed (until you click to another account):

image

This PR updates to show the indicator in the header next to the account name:

image

For any accounts not sync'd (or "All Accounts" etc.) it simply shows no indicator:

image
image

@trafico-bot trafico-bot bot added the 🚧 WIP label Apr 6, 2024
Copy link

netlify bot commented Apr 6, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 820805d
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/6613040dc24e7f00089c1338
😎 Deploy Preview https://deploy-preview-2560.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Apr 6, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
10 4.48 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/BalanceWithCarryover.js 1.43 kB 0%
static/js/AppliedFilters.js 20.35 kB 0%
static/js/narrow.js 216.9 kB 0%
static/js/import.js 105.26 kB 0%
static/js/wide.js 262.04 kB 0%
static/js/ReportRouter.js 1.18 MB 0%
static/js/index.js 2.56 MB 0%

Copy link
Contributor

github-actions bot commented Apr 6, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.2 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.2 MB 0%

@psybers psybers changed the title [WIP] Show sync indicator in account header Show sync indicator in account header Apr 6, 2024
@MatissJanis
Copy link
Member

Cool idea. But we have a small problem: when clicking "edit" the account name jumps to the left. Could we make the edit box stay in the same position as the account name? I believe we could just move the sync indicator outside the /edit/ boundaries and that would solve it.

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

I was aware it 'jumped' left. I assumed, since the box is not sized to the string (try editing a name that is long), that it's moving would be ok. Even with short names, the box is not lined up perfectly with the label so there is a bit of wiggle.

If you think that is a problem though, I can easily fix it.

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

This is the current behavior:

image image

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

(And just to clarify, the behavior I implemented hides the indicator while editing. It is not that the box is on top of it.)

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

I fixed that issue so the indicator stays put and the edit box does not 'jump'.

As a side effect, I also fixed the slight 'wiggle' the box had when showing/hiding (even for accounts without an indicator).

While I am in here would it also make sense to try and have the edit box enlarge to fit its contents for longer names?

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

I could set the input width: accountName.length + "ch" or something similar. Since the font is variable-width, we can't pick a perfect number that sizes it directly to the contents, so I went with the ch unit (the width of a 0 in the font).

Most will have some extra space at the end, as many characters are not as wide as a 0. But it does ensure it is at least as wide as the text.

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

Without changing the width:
image

With changing the width:
image

@psybers
Copy link
Contributor Author

psybers commented Apr 6, 2024

It might make sense to ensure minimum width, e.g. 10 characters width: Math.max(10, accountName.length) + "ch".

@MatissJanis
Copy link
Member

Sorry for the slow answer.

I think this sample is ideally what we would want: #2560 (comment)

If it's not technically overengineered to implement it - would you mind doing it? Otherwise your current solution is also OK and I'm happy to merge it as-is.

@psybers
Copy link
Contributor Author

psybers commented Apr 7, 2024

@MatissJanis No it is a very simple fix. Just pushed.

Copy link
Member

@MatissJanis MatissJanis left a comment

Choose a reason for hiding this comment

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

Thanks! This is great

@MatissJanis MatissJanis merged commit c2291d4 into actualbudget:master Apr 7, 2024
18 of 19 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants