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

Some UI improvements #161

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Some UI improvements #161

wants to merge 12 commits into from

Conversation

brullsker
Copy link

I just changed some things so they look better (in my opinion). I also addressed and hopefully fixed Issue #110 .
I hope you like my contribution. Thanks, brullsker

<RadioButton Content="Name and message" Tag="{x:Bind Vm.NameAndMessageTag}" Margin="0,6" IsChecked="{x:Bind Vm.NameAndMessageChecked, Mode=TwoWay}" Checked="ShowNotificationText_Checked"/>
<RadioButton Content="Name only" Tag="{x:Bind Vm.NameOnlyTag}" Margin="0,6" IsChecked="{x:Bind Vm.NameOnlyChecked, Mode=TwoWay}" Checked="ShowNotificationText_Checked"/>
<RadioButton Content="No name or message" Tag="{x:Bind Vm.NoNameOrMessageTag}" Margin="0,6" IsChecked="{x:Bind Vm.NoNameOrMessageChecked, Mode=TwoWay}" Checked="ShowNotificationText_Checked"/>
<RadioButton Content="No name or message" Tag="{x:Bind Vm.NoNameOrMessageTag}" Margin="0,6" IsChecked="{x:Bind Vm.NoNameOrMessageChecked, Mode=TwoWay}" Checked="ShowNotificationText_Checked"/><!--Does this mean that no notif will be shown?-->
Copy link
Member

Choose a reason for hiding this comment

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

No, this means that if you get a message you'll still get a notification but the person who sent it or the message they sent won't be in the notification.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks

Copy link
Member

@golf1052 golf1052 left a comment

Choose a reason for hiding this comment

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

Looks good, I'll wait for @Trolldemorted to comment as well.

@@ -22,12 +22,12 @@
<RowDefinition Height="Auto"/>
</Grid.RowDefinitions>
<StackPanel>
<TextBlock Text="Show" Style="{StaticResource BodyTextBlockStyle}" Margin="0,0,0,6"/>
<TextBlock Text="Show..." Style="{StaticResource BodyTextBlockStyle}" Margin="0,0,0,6"/>
Copy link
Member

Choose a reason for hiding this comment

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

Ellipsis isn't needed. I tried to match the text from the Signal app, which doesn't use an ellipsis here.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, didn't know that

@@ -29,7 +30,7 @@
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
<TextBlock Text="Settings" Style="{StaticResource HeaderTextBlockStyle}" HorizontalAlignment="Center" VerticalAlignment="Center"/>
<VariableSizedWrapGrid x:Name="WrapGrid" HorizontalAlignment="Left" MaximumRowsOrColumns="1" VerticalAlignment="Top" Orientation="Horizontal" ItemWidth="250" ItemHeight="80" Grid.Row="1" Margin="32,0,32,0">
<VariableSizedWrapGrid x:Name="WrapGrid" HorizontalAlignment="Left" MaximumRowsOrColumns="1" VerticalAlignment="Top" Orientation="Horizontal" ItemWidth="250" ItemHeight="80" Grid.Row="1" Margin="32,0,32,0" animations:ReorderGridAnimation.Duration="300">
Copy link
Member

Choose a reason for hiding this comment

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

This animation seems to not work for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't notice a difference on my W10 desktop.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, seems to not work... Sorry

Copy link
Contributor

@Trolldemorted Trolldemorted left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request!

I have added some comments for minor issues.

@@ -132,7 +132,7 @@
<ColumnDefinition Width="*"/>
<ColumnDefinition Width="Auto"/>
</Grid.ColumnDefinitions>
<TextBox Grid.Column="0" Name="InputTextBox" VerticalAlignment="Center" KeyDown="TextBox_KeyDown" PlaceholderText="Type a message" BorderBrush="{x:Null}" BorderThickness="0" TextWrapping="Wrap" TextChanged="InputTextBox_TextChanged" InputScope="Chat" Visibility="{x:Bind SendMessageVisible, Mode=OneWay}"/>
<TextBox Grid.Column="0" Name="InputTextBox" VerticalAlignment="Center" KeyDown="TextBox_KeyDown" PlaceholderText="Type a message" BorderBrush="{x:Null}" BorderThickness="0" TextWrapping="Wrap" TextChanged="InputTextBox_TextChanged" InputScope="Chat" Visibility="{x:Bind SendMessageVisible, Mode=OneWay}" Padding="10,15,6,15"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous (default) padding was 10,3,6,5. Do other chat apps on uwp also use such a big padding to north and south? If the input becomes multiline we might want to waste as little vertical space as possible

Copy link
Author

Choose a reason for hiding this comment

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

Well, changing padding was the first thing coming to my mind...

<Image Source="ms-appx:///Assets/logo-idea-2.png" Width="256"/>
<StackPanel Orientation="Vertical">
<Image Source="ms-appx:///Assets/logo-idea-2.png" Width="256"/>
<TextBlock Text="Choose a contact to start chatting" Grid.Row="1" HorizontalAlignment="Center" Style="{StaticResource TitleTextBlockStyle}" Foreground="White" Margin="0,12,0,0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Contact or group, maybe we should just say "conversation"?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, sounds better

<StackPanel Grid.Row="1" Margin="32,0,0,0" HorizontalAlignment="Left">
<HyperlinkButton x:Name="ExportDebugLogButton" Content="Export debug log" Click="ExportDebugLogButton_Click" HorizontalAlignment="Stretch" HorizontalContentAlignment="Left"/>
<HyperlinkButton x:Name="SyncButton" Content="Request contact and group sync" Margin="0,8,0,0" Click="SyncButton_Click" HorizontalAlignment="Stretch" HorizontalContentAlignment="Left"/>
<HyperlinkButton x:Name="TestCrashButton" Content="Intentionally crash the app to test its debug log export" Click="TestCrashButton_Click" HorizontalAlignment="Stretch" HorizontalContentAlignment="Left"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why HyperlinkButtons? Do they behave/look different here?

@@ -29,7 +30,7 @@
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
<TextBlock Text="Settings" Style="{StaticResource HeaderTextBlockStyle}" HorizontalAlignment="Center" VerticalAlignment="Center"/>
<VariableSizedWrapGrid x:Name="WrapGrid" HorizontalAlignment="Left" MaximumRowsOrColumns="1" VerticalAlignment="Top" Orientation="Horizontal" ItemWidth="250" ItemHeight="80" Grid.Row="1" Margin="32,0,32,0">
<VariableSizedWrapGrid x:Name="WrapGrid" HorizontalAlignment="Left" MaximumRowsOrColumns="1" VerticalAlignment="Top" Orientation="Horizontal" ItemWidth="250" ItemHeight="80" Grid.Row="1" Margin="32,0,32,0" animations:ReorderGridAnimation.Duration="300">
Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't notice a difference on my W10 desktop.

@@ -32,19 +32,22 @@
<RowDefinition Height="Auto" />
<RowDefinition Height="*" />
</Grid.RowDefinitions>
<Border Grid.Row="0" Background="{ThemeResource ApplicationPageBackgroundThemeBrush}"/>
<Border Grid.Row="0" Background="Black" Opacity="0.25"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The blue color was exactly the same blue the official signal clients use at that position - do we want to diverge color-wise? I think we should keep the general look so that S-W feels like an official signal client

Copy link
Author

Choose a reason for hiding this comment

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

Do you think it would look better without that black-transparent border?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know tbh, let's just stick with Signal-Android/Desktops color schemes

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.

3 participants