Frontend.XF: added the settings icon and side menu #203

kubaflo · 2023-01-30T14:07:21Z

iOS	Android

knocte · 2023-01-30T16:59:59Z

@kubaflo why do you include the two PNG logo files? there are already logo files in the iOS project

knocte · 2023-01-30T17:00:29Z

there are already logo files in the iOS project

Here: https://github.com/nblockchain/geewallet/tree/frontend/src/GWallet.Frontend.XF.iOS/Assets.xcassets/AppIcon.appiconset

knocte · 2023-01-30T17:01:14Z

src/GWallet.Frontend.XF/GWallet.Frontend.XF.fsproj

@@ -141,5 +144,6 @@
    <PackageReference Include="Fsdk" Version="0.6.0--date20230118-0634.git-b829db2">
      <GeneratePathProperty></GeneratePathProperty>
    </PackageReference>
+    <EmbeddedResource Include="Resources\Fonts\FontAwesomeSolid.otf" />


@kubaflo let's remove this, we can improve the design in a separate PR

knocte · 2023-01-30T17:02:00Z

src/GWallet.Frontend.XF/BalancesPage.xaml.fs

+        this.IsPresented <- true
+
+    member this.OpenSettings_Tapped(_: obj, _: EventArgs) =
+        Application.Current.MainPage.DisplayAlert("Settings", "Succesfully opened the settings page", "Ok") |> ignore


@kubaflo let's hook up the real feature, same as what Frontend.Console is doing, please

But isn't it a request beyond the scope of this task? I thought I was supposed to add an icon and a side menu, not implement the things in the settings inside the app. I can do it but I don't think that it should be done in this PR

There are things that can be split to other PRs and there are things that cannot. If I merge this PR as is, then the frontend branch would be in an unreleasable state, you understand? We cannot show to end-users certain UI that does nothing.

@knocte okay, I will implement it

@knocte can I use popups to implement this functionality or you'd prefer separate pages?

it depends; if popups mean a lot of platform-specific code, I'd prefer pages

kubaflo · 2023-01-31T00:32:12Z

@kubaflo why do you include the two PNG logo files? there are already logo files in the iOS project

Fixed

src/GWallet.Frontend.XF/GWallet.Frontend.XF.fsproj

knocte · 2023-01-31T03:28:41Z

src/GWallet.Frontend.XF/BalancesPage.xaml

             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             xmlns:local="clr-namespace:GWallet.Frontend.XF"
+             xmlns:ios="clr-namespace:Xamarin.Forms.PlatformConfiguration.iOSSpecific;assembly=Xamarin.Forms.Core"


what's this iOS thing?

Without we wouldn't be able to apply a safe area to the ios version of the app

what is a safe area?

It's an area where the notch is. Here are the screens how the app would look like without it

knocte · 2023-01-31T05:16:20Z

This branch cannot be rebased due to conflicts

What happened here? (I generally recommend rebasing PRs via git rebase command, not git merge.)

kubaflo · 2023-02-03T23:40:06Z

iOS
Android

@knocte This is how it looks now :)

kubaflo · 2023-02-05T14:48:34Z

@knocte I did the rebase, squashed commits, removed unnecessary things and removed colours

knocte · 2023-02-05T15:18:00Z

@knocte I did the rebase,

And you also squashed commits! Nice.

But CI is red because of this warning:

geewallet/src/GWallet.Frontend.XF/SettingsPage.xaml.fs(58,17): error FS0020: The result of this expression has type 'Async<unit>' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'. [/__w/geewallet/geewallet/src/GWallet.Frontend.XF/GWallet.Frontend.XF.fsproj]

We have warnings as errors in Release mode :)

kubaflo · 2023-02-05T15:50:52Z

@knocte I did the rebase,

And you also squashed commits! Nice.

But CI is red because of this warning:

geewallet/src/GWallet.Frontend.XF/SettingsPage.xaml.fs(58,17): error FS0020: The result of this expression has type 'Async<unit>' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'. [/__w/geewallet/geewallet/src/GWallet.Frontend.XF/GWallet.Frontend.XF.fsproj]

We have warnings as errors in Release mode :)

@knocte I fixed this error. However, if CI after this is still red, I will need your help :)

knocte · 2023-02-06T02:58:53Z

src/GWallet.Frontend.XF/SettingsPage.xaml

+    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
+    x:Class="GWallet.Frontend.XF.SettingsPage">
+
+    <AbsoluteLayout>


@kubaflo question, is there any specific reason of why you used AbsoluteLayout here instead of an non-absolute alternative?

Yes, it is necessary as I want the 'mainLayout' to occupy 70% of the page's width

<StackLayout x:Name="mainLayout" AbsoluteLayout.LayoutFlags="XProportional,SizeProportional" AbsoluteLayout.LayoutBounds="0.5,50,0.7,1" VerticalOptions="FillAndExpand">

knocte · 2023-02-08T15:36:45Z

src/GWallet.Frontend.XF/BalancesPage.xaml

+                <Image HeightRequest="80"
+                       WidthRequest="80"
+                       Margin="0,0,0,10"
+                       x:Name="logoInSidemenu"/>


@kubaflo cosmetic: let's rename this to sideMenuImg (this way, in case we decide to stop using a logo in the future, we don't need to rename it again)

knocte · 2023-02-08T15:38:27Z

src/GWallet.Frontend.XF/BalancesPage.xaml

+                    <Label VerticalOptions="Center" Text="#"/>
+                    <Label Text="Check you still remember your payment password"/>
+                    <StackLayout.GestureRecognizers>
+                        <TapGestureRecognizer CommandParameter="Check you still remember your payment password" Tapped="OpenSettings_Tapped"/>


@kubaflo 2 cosmetic things here:

Let's not use underscores in method names

We don't need to put the full sentence here in the CommandParameter attribute right? I mean, in case we need to change the text of the UI in the future, we should be able to just change it in one place, not three

knocte · 2023-02-08T15:38:50Z

src/GWallet.Frontend.XF/BalancesPage.xaml

+                    <Label VerticalOptions="Center" Text="#"/>
+                    <Label Text="Check you still remember your secret recovery phrase"/>
+                    <StackLayout.GestureRecognizers>
+                        <TapGestureRecognizer CommandParameter="Check you still remember your secret recovery phrase" Tapped="OpenSettings_Tapped"/>


@kubaflo same 2 cosmetic things here ^

knocte · 2023-02-08T15:39:05Z

src/GWallet.Frontend.XF/BalancesPage.xaml

+                    <Label VerticalOptions="Center" Text="#"/>
+                    <Label Text="Wipe your current wallet, in order to start from scratch"/>
+                    <StackLayout.GestureRecognizers>
+                        <TapGestureRecognizer CommandParameter="Wipe your current wallet, in order to start from scratch" Tapped="OpenSettings_Tapped"/>


@kubaflo same 2 cosmetic things here ^

knocte · 2023-02-08T15:40:55Z

src/GWallet.Frontend.XF/BalancesPage.xaml

-            <controls:CircleChartView x:Name="normalChartView"
-                                     HorizontalOptions="FillAndExpand"
-                                     VerticalOptions="FillAndExpand"/>
+                <controls:CircleChartView x:Name="normalChartView"


@kubaflo cosmetic: do you mind moving all the XML fragment that you didn't modify, to the left one level of indentation? I know that your change in the XAML should cause this XML fragment to move to the right, but I'd prefer if do proper formatting of the XML in a 2nd commit (I'm planning to use an automatic XML formatter for that)

@kubaflo (this way, this part will not show up in the diff)

knocte · 2023-02-08T15:42:37Z

src/GWallet.Frontend.XF/BalancesPage.xaml.fs

@@ -46,6 +46,8 @@ type BalancesPage(state: FrontendHelpers.IGlobalAppState,
    let contentLayout = base.FindByName<StackLayout> "contentLayout"
    let normalChartView = base.FindByName<CircleChartView> "normalChartView"
    let readonlyChartView = base.FindByName<CircleChartView> "readonlyChartView"
+    let logoInSideMenu = base.FindByName<Image> "logoInSidemenu"


@kubaflo as we decided to rename the image name to sideMenuImg, let's also change the variable name here ^

knocte · 2023-02-09T06:36:29Z

src/GWallet.Frontend.XF/SettingsPage.xaml.fs

+    //loading & result
+    let resultMessage = base.FindByName<Label> "resultMessage"
+    let loadingIndicator = base.FindByName<ActivityIndicator> "loadingIndicator"
+    let option = option


@kubaflo this line above is not needed

knocte · 2023-02-09T06:37:23Z

src/GWallet.Frontend.XF/BalancesPage.xaml

-                                     VerticalOptions="FillAndExpand"/>
+                                        IsVisible="False"
+                                        HorizontalOptions="FillAndExpand"
+                                        VerticalOptions="FillAndExpand"/>


@kubaflo see above ^ this space additions are not needed

knocte · 2023-02-09T18:08:41Z

We have fixed the check-password performance problem, if you rebase this PR then we can test the improvement with the settings page.

kubaflo · 2023-02-09T22:54:33Z

We have fixed the check-password performance problem, if you rebase this PR then we can test the improvement with the settings page.

It works!!!

Added the settings page. Added the settings icon and side menu.

knocte · 2023-10-16T05:12:03Z

The closing of this PR was unintended (it happened because the 'frontend' branch has now disappeared). @aarani can you please re-create this PR as it was, but targetting the master branch now? Thanks

aarani · 2023-10-16T07:50:15Z

Superseded by #233

kubaflo marked this pull request as ready for review January 30, 2023 14:08

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from 9bd5499 to c6179db Compare January 30, 2023 14:30

kubaflo changed the title ~~Added the settings icon and side menu~~ Frontend.XF: Added the settings icon and side menu Jan 30, 2023

knocte reviewed Jan 30, 2023

View reviewed changes

knocte reviewed Jan 31, 2023

View reviewed changes

src/GWallet.Frontend.XF/GWallet.Frontend.XF.fsproj Outdated Show resolved Hide resolved

knocte reviewed Jan 31, 2023

View reviewed changes

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from 68a96c8 to 3901e73 Compare February 5, 2023 14:45

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch 3 times, most recently from f575e81 to 87ff387 Compare February 5, 2023 15:06

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch 2 times, most recently from 73e25ec to 8327249 Compare February 5, 2023 15:40

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from 8327249 to 440e0bb Compare February 5, 2023 16:27

knocte reviewed Feb 6, 2023

View reviewed changes

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from 440e0bb to dadd3d3 Compare February 6, 2023 12:23

knocte reviewed Feb 8, 2023

View reviewed changes

knocte reviewed Feb 9, 2023

View reviewed changes

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch 2 times, most recently from 07f32ce to e8cfa47 Compare February 9, 2023 15:57

kubaflo changed the title ~~Frontend.XF: Added the settings icon and side menu~~ Frontend.XF: added the settings icon and side menu Feb 9, 2023

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from e8cfa47 to 9f5e79c Compare February 9, 2023 22:52

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch 7 times, most recently from 4096cd4 to 99cd00e Compare February 15, 2023 13:05

Frontend.XF: added the side menu & settings page

8d64c2b

Added the settings page. Added the settings icon and side menu.

kubaflo force-pushed the Frontend.XF-Added-Settings-UI branch from 99cd00e to 8d64c2b Compare March 2, 2023 14:47

knocte force-pushed the frontend branch 8 times, most recently from a02943f to 7a94774 Compare August 12, 2023 03:03

knocte deleted the branch nblockchain:frontend October 15, 2023 13:54

knocte closed this Oct 15, 2023

aarani mentioned this pull request Oct 16, 2023

[UNTESTED] Frontend.XF: added the side menu & settings page #233

Open

This pull request was closed.

Frontend.XF: added the settings icon and side menu #203

Frontend.XF: added the settings icon and side menu #203

Conversation

kubaflo commented Jan 30, 2023 • edited Loading

knocte commented Jan 30, 2023

knocte commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubaflo commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubaflo Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

knocte commented Jan 31, 2023

kubaflo commented Feb 3, 2023

kubaflo commented Feb 5, 2023 • edited Loading

knocte commented Feb 5, 2023

kubaflo commented Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

kubaflo Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

knocte Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knocte commented Feb 9, 2023

kubaflo commented Feb 9, 2023

knocte commented Oct 16, 2023

aarani commented Oct 16, 2023

kubaflo commented Jan 30, 2023 •

edited

Loading

kubaflo Jan 31, 2023 •

edited

Loading

kubaflo commented Feb 5, 2023 •

edited

Loading

kubaflo commented Feb 5, 2023 •

edited

Loading

kubaflo Feb 6, 2023 •

edited

Loading

knocte Feb 8, 2023 •

edited

Loading