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

Replace circle chart by hoop chart #234

Closed
wants to merge 4 commits into from
Closed

Replace circle chart by hoop chart #234

wants to merge 4 commits into from

Conversation

webwarrior-ws
Copy link
Contributor

Addresses #172 (Better design for cheese chart (so that text is inside it)).
Replaces #209.

@webwarrior-ws
Copy link
Contributor Author

Commit that adds Maui compatibility is omitted from this PR as master doesn't have Maui support.

@knocte
Copy link
Member

knocte commented Oct 16, 2023

Commit that adds Maui compatibility is omitted from this PR as master doesn't have Maui support.

Please add a link to that commit from here, for the record.

@webwarrior-ws
Copy link
Contributor Author

Commit that adds Maui compatibility is omitted from this PR as master doesn't have Maui support.

Please add a link to that commit from here, for the record.

webwarrior-ws@552c711



type private HoopChartState =
| Uninitialized
Copy link
Member

Choose a reason for hiding this comment

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

@webwarrior-ws remind us why do you need this DU member? <- @aarani pls review this bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC this is because control is created before data sources are set for it.

Copy link
Member

Choose a reason for hiding this comment

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

why does the circleChart not need this then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Because property for default image may not be yet set

Let's set it earlier then?

Copy link
Member

Choose a reason for hiding this comment

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

so why CircleChartView doesn't need Uninitialized thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably because HoopChart does layout itself, and must know its size, while old CircleChartView just replaced its Content property with either chart grid or image.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC A view is rendered multiple times before the parameters get fully propagated so Uninitialized state might be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Cannot we have an invisible image for the logo that only turns into visible if balances are zero?

Copy link
Contributor

@aarani aarani Oct 16, 2023

Choose a reason for hiding this comment

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

Cannot we have an invisible image for the logo that only turns into visible if balances are zero?

I don't know, tbh.

webwarrior and others added 2 commits October 16, 2023 12:28
Workaround for Android when certain small arcs wouldn't render.
If arc lengthis very small, draw a dot instead.
Change `this` identifier to `self` to follow coding guidelines.
@Mersho
Copy link
Contributor

Mersho commented Feb 22, 2024

LGTM! 🚀 However, I encountered a problem when attempting to install it on the Android emulator using JetBrains Rider (windows):

  Error parsing the project "Fsdk" section with GUID: "{6EE07541-91A1-42C2-A21F-2809BBDC2F50}". It is nested under "{CE79AB9D-0FB9-4606-B7C6-A78C0858CC54}" but that project is not found in the solution.

The issue was resolved by deleting certain lines from the geewallet.sln file:

	EndGlobalSection
	GlobalSection(NestedProjects) = preSolution
		{6EE07541-91A1-42C2-A21F-2809BBDC2F50} = {CE79AB9D-0FB9-4606-B7C6-A78C0858CC54}
	EndGlobalSection

@Mersho
Copy link
Contributor

Mersho commented Feb 22, 2024

During my testing on the ubuntu 22.04 vm, I observed several issues in UI:

  • The color indicators for each currency are absent.
  • The wallet balance should be centered within the circle. Perhaps the chart could expand when an additional digit is added to the balance.
image

Fix wrong total balance label layout on GTK.
@knocte knocte force-pushed the master branch 2 times, most recently from b841302 to ccb1641 Compare May 8, 2024 09:18
@webwarrior-ws webwarrior-ws closed this by deleting the head repository May 23, 2024
This pull request was closed.
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.

4 participants