Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dashboard Page & Group Colors #393

Merged
merged 104 commits into from
Apr 7, 2023
Merged

Dashboard Page & Group Colors #393

merged 104 commits into from
Apr 7, 2023

Conversation

nlogozzo
Copy link
Member

@nlogozzo nlogozzo commented Mar 20, 2023

Closes #162
Closes #339
Closes #407

TODO

  • Redesigned AccountViewPage for WinUI to better match fluent design
  • Group Colors
    • Backend
    • GNOME
    • WinUI
    • All: TransactionDialog -> Change To Group Color When Group Selected
    • Shared: Ungrouped color match config
  • DashboardViewController
  • GNOME Implementation
  • WinUI Implementation
  • Fix password dialog in GNOME

@nlogozzo
Copy link
Member Author

nlogozzo commented Mar 25, 2023

Redesigned AccountViewPage for WinUI to better match fluent design

Previous WinUI design was something that was a directly design copy from GNOME. While the design works really well on GNOME, it did not on WinUI. It felt very congested and at times complicated to use, so I redid it! Furthermore, in keeping with WinUI design, I moved the tab view from the top of the application to a navigation sidebar.
Here is the new redesigned, WinUI design:

image
image
image
image
image

The sort flyout is a little broken...fixing it now

@nlogozzo nlogozzo changed the title Dashboard (Accounts Overview) Page Dashboard (Accounts Overview) Page & Group Colors Mar 26, 2023
@nlogozzo nlogozzo changed the title Dashboard (Accounts Overview) Page & Group Colors Dashboard Page & Group Colors Mar 26, 2023
@nlogozzo
Copy link
Member Author

nlogozzo commented Apr 7, 2023

WinUI done :)
On WinUI, if the Ungrouped group is selected I have the drop-down hidden and i just show the color button.
Maybe you can do the same on GNOME. Or if the Ungrouped group is selected maybe have the drop-down say, use default color or use unique color. Because when the Ungrouped group is selected, it will use the default transaction color and not the group color.

@fsobolev
Copy link
Member

fsobolev commented Apr 7, 2023

WinUI done :) On WinUI, if the Ungrouped group is selected I have the drop-down hidden and i just show the color button. Maybe you can do the same on GNOME. Or if the Ungrouped group is selected maybe have the drop-down say, use default color or use unique color. Because when the Ungrouped group is selected, it will use the default transaction color and not the group color.

What if a user wants to treat "Ungrouped" as regular group (and in fact it is a regular group, it also has a color) and assign some transactions to it also wishing for these transactions to use the group's color (= default group color)? I don't see a point in disallowing this.
But this is probably will be not very common workflow, using default transaction color would be preferable I think. And this is easy to change: UseGroupColor should be false by default (currently it's true).

@nlogozzo
Copy link
Member Author

nlogozzo commented Apr 7, 2023

What if a user wants to treat "Ungrouped" as regular group (and in fact it is a regular group, it also has a color) and assign some transactions to it also wishing for these transactions to use the group's color (= default group color)?

At this point, it removes the purpose of Default Transaction Color since everything will use default group color then, unless the user specifies something different, hence why I was saying to change the wording to just use default color when the selected group is Ungrouped.

But this is probably will be not very common workflow, using default transaction color would be preferable I think. And this is easy to change: UseGroupColor should be false by default (currently it's true).

If the group is set to Ungrouped it currently does uase the default transaction color as we have it where if the groupid is <= 0 (0 being ungrouped) use transaction color else group color. So having default UseGroupColor be false would have no effect. I think leaving it as true is fine.

@fsobolev
Copy link
Member

fsobolev commented Apr 7, 2023

At this point, it removes the purpose of Default Transaction Color

Not really. Default transaction color can be used for specific transactions no matter what the group is. It's kinda creating a "group" of transactions without actually changing their groups, just by marking them visually. And using default transaction color for this can save a few clicks.

If the group is set to Ungrouped it currently does uase the default transaction color as we have it where if the groupid is <= 0 (0 being ungrouped) use transaction color else group color.

Does it? Well... Again, I don't see a point in disallowing using the color of "Ungrouped" group for transactions. I think this should be changed.

This reverts commit 5dc9912.
@fsobolev
Copy link
Member

fsobolev commented Apr 7, 2023

About this commit: 8748368
No matter whether we want or not to have UseGroupColor set to true for new transactions, it should be false when creating an entry in database for backward-compatibility. Imagine what will happen when users will update from 2023.2.x to 2023.4.0 — they will have all transactions green. Because all groups will have green color and all transactions will follow it.

@fsobolev
Copy link
Member

fsobolev commented Apr 7, 2023

I don't see a point in disallowing this.

I found a point: it would be very rare workflow that doesn't worth increasing code complexity that would be required to pass default group color everywhere. So I made changed to match WinUI as you said. Color row is now not ComboRow, but ActionRow with DropDown that have a popover that looks a bit different than a popover of ComboRow (different position and arrow)... But I hope nobody will notice 😅

@nlogozzo
Copy link
Member Author

nlogozzo commented Apr 7, 2023

Looks good to me!

@nlogozzo
Copy link
Member Author

nlogozzo commented Apr 7, 2023

If nothing else, you can approve the PR and we can merge :)

@nlogozzo nlogozzo merged commit cfda57c into main Apr 7, 2023
@nlogozzo nlogozzo deleted the dashboard branch April 7, 2023 19:39
@nlogozzo
Copy link
Member Author

nlogozzo commented Apr 7, 2023

Yay 🥳

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove scroll on "insert password" window Group colors Opened Accounts Overview Page
2 participants