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

Wip/hoop chart #173

Closed
wants to merge 14 commits into from
Closed

Wip/hoop chart #173

wants to merge 14 commits into from

Conversation

webwarrior-ws
Copy link
Contributor

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

@knocte
Copy link
Member

knocte commented Jul 4, 2022

@webwarrior-ws good work! Can you enable GitHubActions in your fork please?


let balanceTagLabel =
Label(
Text = "Amount Balance",
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 actually let's have the same text as the old label: Total Assets:\n

@webwarrior-ws
Copy link
Contributor Author

Changed label text. CI job failure seems to be unrelated.

@knocte
Copy link
Member

knocte commented Jul 6, 2022

CI job failure seems to be unrelated.

True, it was unrelated, but do you mind rebasing your PR? I fixed the CI issue in wip/hoopChart branch.

@webwarrior-ws
Copy link
Contributor Author

CI job failure seems to be unrelated.

True, it was unrelated, but do you mind rebasing your PR? I fixed the CI issue in wip/hoopChart branch.

I must have done something wrong, because now I can't push:

Pushing to https://github.com/webwarrior-ws/geewallet
To https://github.com/webwarrior-ws/geewallet
! [remote rejected] wip/hoopChart -> wip/hoopChart (refusing to allow an OAuth App to create or update workflow > .github/workflows/CI.yml without workflow scope)
error: failed to push some refs to 'https://github.com/webwarrior-ws/geewallet'

@knocte
Copy link
Member

knocte commented Jul 6, 2022

Did you forget the flag --force?

@knocte
Copy link
Member

knocte commented Jul 6, 2022

If that doesn't do it; it seems it's the way you cloned your fork? I recommend you to use SSH, not weird OAuth stuff.

@webwarrior-ws
Copy link
Contributor Author

OK, now with SSH it worked

@knocte
Copy link
Member

knocte commented Jul 6, 2022

I said rebase, not merge :(

When rebasing, there should not be any merge commits on top of your branch.

@webwarrior-ws
Copy link
Contributor Author

I said rebase, not merge :(

When rebasing, there should not be any merge commits on top of your branch.

So now what? Do I checkout or reset to latest commit that is not mine?

@knocte
Copy link
Member

knocte commented Jul 6, 2022

IMO you should first delete your branch locally, then checkout commit 9e70c43 and create a new branch from it (same name as the one that this PR is targetting), then cherry-pick your commits from github, then push with --force.

webwarrior added 4 commits July 6, 2022 14:52
Also removed no longer used CircleChartView.fs file
#172

(cherry picked from commit f4219b9)
Made Percent field of Percentage record immutable

(cherry picked from commit 761484c)
@webwarrior-ws webwarrior-ws requested a review from knocte July 7, 2022 09:57
@knocte
Copy link
Member

knocte commented Jul 7, 2022

Finally, I had time to test it today (sorry, yesterday I was in the eye-doctor and they gave me something that made my sight blurry).

Code-wise, the proposed PR looks fine; integration seems clean. But I have this rendering problem:

Screenshot 2022-07-07 at 9 03 13 PM

As you can see, the circle is too big, and causes the currency frames to need scrolling. Ideally the circle shouldn't be rendered as big and everything should fit. Do you know if this is easy to fix?

@webwarrior-ws
Copy link
Contributor Author

Finally, I had time to test it today (sorry, yesterday I was in the eye-doctor and they gave me something that made my sight blurry).

Code-wise, the proposed PR looks fine; integration seems clean. But I have this rendering problem:

As you can see, the circle is too big, and causes the currency frames to need scrolling. Ideally the circle shouldn't be rendered as big and everything should fit. Do you know if this is easy to fix?

Probably not.
In order to do that, shapes that make up hoop chart should resize to fill the container. And honestly I don't know how shapes and paths in Xamarin.Froms work.
Also the chart can't be allowed to get too small, because label might overlap with the shapes.

@knocte
Copy link
Member

knocte commented Jul 7, 2022

Also the chart can't be allowed to get too small, because label might overlap with the shapes.

Exactly! But the idea is that the circle only gets big when it needs to (because of a big label). In the case of the screenshot, the amount is not very long, so the circle should not need to be that big.

@knocte
Copy link
Member

knocte commented Jul 26, 2022

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.

2 participants