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.

Sign up for GitHub

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

Jump to bottom

Fix OpenFile, SaveFile, Directory pickers style #622

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix OpenFile, SaveFile, Directory pickers style #622

wants to merge 5 commits into from

Conversation

Dmitry-VF
Copy link
Contributor

@Dmitry-VF Dmitry-VF commented Apr 21, 2023 •
edited
Loading

Description of Change

qQJbTqWXMx

a9OvmnIge3

ZqOboi23Dd

Fix styles to make it look more even

Issues Resolved

  • fixes #

API Changes

None

Platforms Affected

  • All
  • WPF
  • UWP
  • iOS
  • Android

Behavioral Changes

None

Testing Procedure

PR Checklist

  • I have included examples or tests
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@Dmitry-VF Dmitry-VF requested review from mkhomutov and ceo1647 April 21, 2023 19:49
@Dmitry-VF Dmitry-VF self-assigned this Apr 21, 2023
@ceo1647
Copy link
Contributor

ceo1647 commented Apr 22, 2023

I found a problem with margins in SaveFilePicker (but i think the same problem may be in Open/Directory pickers.. need to check) If labelWidth = 0
image
looks like the margins on label should be 0 when LabelWidth is 0

@ceo1647
Copy link
Contributor

ceo1647 commented Apr 22, 2023

another one is we have to add DefaultOpenFile/Directory/SaveFilePickerStyle
image
and set default margins ...also i recommend set Height to {DynamicResource Size.Height}; like here:
image
but in a Default styles as for all other controls

@ceo1647
Copy link
Contributor

ceo1647 commented Apr 22, 2023

image

@ceo1647
Copy link
Contributor

ceo1647 commented Apr 22, 2023

Suggestion, maybe it's better to set default LabelWidth value to 0... as for me it's ugly solution, because you'll never get it right:
image
It's better to have Label separately from Picker control, then it's easier to adjust margins in a grid

ceo1647
ceo1647 requested changes Apr 22, 2023
View reviewed changes
src/Orc.Controls/Controls/SaveFilePicker/Themes/SaveFilePicker.generic.xaml Outdated
@@ -3,5 +3,8 @@
xmlns:controls="clr-namespace:Orc.Controls">

<Style x:Key="{x:Type controls:SaveFilePicker}"
TargetType="{x:Type controls:SaveFilePicker}"/>
TargetType="{x:Type controls:SaveFilePicker}">
<Setter Property="Margin" Value="{DynamicResource Margin.TextBox}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

move this setter to DefaultSaveFilePicker (See overall comments)

src/Orc.Controls/Controls/OpenFilePicker/Themes/OpenFilePicker.generic.xaml Outdated

<Style x:Key="{x:Type controls:OpenFilePicker}"
TargetType="{x:Type controls:OpenFilePicker}">
<Setter Property="Margin" Value="{DynamicResource Margin.TextBox}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

move this setter to DefaultOpenFilePickerStyle (See overall comments)

src/Orc.Controls/Controls/DirectoryPicker/Themes/DirectoryPicker.generic.xaml Outdated

<Style x:Key="{x:Type controls:DirectoryPicker}"
TargetType="{x:Type controls:DirectoryPicker}">
<Setter Property="Margin" Value="{DynamicResource Margin.TextBox}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

move this setter to DefaultDirectoryPickerStyle (See overall comments)

src/Orc.Controls/Controls/OpenFilePicker/Views/OpenFilePicker.xaml Outdated

<TextBox AutomationProperties.AutomationId="{x:Static automation:OpenFilePickerMap.SelectedFileTextBoxId}"
Grid.Column="1"
IsReadOnly="True"
Text="{Binding SelectedFileDisplayName, Mode=OneWay}"
Height="{Binding ElementName=OpenFileButton, Path=ActualHeight}"/>
Height="{Binding ElementName=OpenFileButton, Path=ActualHeight}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove it (and also remove it from other pickers, if they have it too) Binding to sizes 95% in cases is wrong solution. I understand that maybe it's not your code, but as long as we noticed it - we should get rid of it: please try to remove it or use VerticalAlignment=Stretch -> whatever will work

@ceo1647
Copy link
Contributor

ceo1647 commented Apr 22, 2023

Another suggestion, but for other task -> as we working on OpenFilePicker/SaveFilePicker/Directory picker ... there is a lot of extra code here because we use MVVM approach here and controls are not extendable (changed control template, styles, attached events etc); so created issue for it https://sesolutions.atlassian.net/browse/ORCOMP-657

@Dmitry-VF Dmitry-VF requested a review from ceo1647 April 24, 2023 17:21
ceo1647
ceo1647 reviewed Apr 24, 2023
View reviewed changes
src/Orc.Controls/Controls/OpenFilePicker/Views/OpenFilePicker.xaml.cs Outdated
@@ -25,6 +25,7 @@ public OpenFilePicker()
}

#region Properties
[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Controls/OpenFilePicker/Views/OpenFilePicker.xaml.cs Outdated
@@ -33,9 +34,11 @@ public double LabelWidth
}

// Using a DependencyProperty as the backing store for LabelWidth. This enables animation, styling, binding, etc...
[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Controls/OpenFilePicker/Views/OpenFilePicker.xaml.cs Outdated

[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Controls/OpenFilePicker/Views/OpenFilePicker.xaml.cs Outdated
@@ -44,6 +47,7 @@ public string LabelText
}

// Using a DependencyProperty as the backing store for LabelText. This enables animation, styling, binding, etc...
[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Controls/SaveFilePicker/Views/SaveFilePicker.xaml.cs Outdated
@@ -24,24 +24,27 @@ public SaveFilePicker()
InitializeComponent();
}

[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Controls/SaveFilePicker/Views/SaveFilePicker.xaml.cs Outdated
[ViewToViewModel(MappingType = ViewToViewModelMappingType.TwoWayViewWins)]
public string LabelText
{
get { return (string)GetValue(LabelTextProperty); }
set { SetValue(LabelTextProperty, value); }
}

[ObsoleteEx(Message = "This property will be deprecated", TreatAsErrorFromVersion ="4.9.0", RemoveInVersion = "6.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.

TreatAsErrorFromVersion ="5.0.0"

src/Orc.Controls/Themes/Generic.generated.xaml Outdated
@@ -2,6 +2,8 @@
<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:local="clr-namespace:Orc.Controls" xmlns:catel="http://schemas.catelproject.com" xmlns:automation="clr-namespace:Orc.Controls.Automation" xmlns:sys="clr-namespace:System;assembly=mscorlib" xmlns:converters="clr-namespace:Orc.Controls.Converters" xmlns:xamlbehaviors="http://schemas.microsoft.com/xaml/behaviors" xmlns:orctheming="http://schemas.wildgums.com/orc/theming">
<ResourceDictionary.MergedDictionaries>
<ResourceDictionary Source="/Orc.Theming;component/themes/generic.xaml" />
<ResourceDictionary Source="/Orc.Controls;component/Controls/DirectoryPicker/Themes/DirectoryPicker.generic.xaml" />
Copy link
Contributor

Choose a reason for hiding this comment

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

looks as it something wrong with generated xaml

Copy link
Contributor

Choose a reason for hiding this comment

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

it should not be merged dictionaries ...because generated.xaml itself solves a problem with merged dictionaries

src/Orc.Controls/Themes/Generic.generated.xaml Outdated
@@ -1481,6 +1482,9 @@
</Setter.Value>
</Setter>
</Style>
<Style x:Key="{x:Type local:SaveFilePicker}" TargetType="{x:Type local:SaveFilePicker}">
<Setter Property="Margin" Value="{DynamicResource Margin.TextBox}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also strange...Margin should be only on Default***Style

src/Orc.Controls/Controls/DirectoryPicker/Views/DirectoryPicker.xaml
<Label.Style>
<Style TargetType="Label">
<Style.Triggers>
<DataTrigger Binding="{Binding LabelText, Converter={catel:EmptyStringToCollapsingVisibilityConverter}}">
Copy link
Contributor

Choose a reason for hiding this comment

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

how does the trigger works without Value property specified? (Same in other cases)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to use DataTrigger...why not use Content property and simple Trigger? (but as for me it's not critical as long as it would be deprecated)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another one: what if LabelWidth would be 0 -> what kind of margin it would be?

src/Orc.Controls/Controls/DirectoryPicker/Views/DirectoryPicker.xaml
<Setter Property="Margin" Value="0"/>
</DataTrigger>
</Style.Triggers>
</Style>
Copy link
Contributor

Choose a reason for hiding this comment

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

please re-style the code

@GeertvanHorrik
Copy link
Member

The buttons don't seem square?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ceo1647 ceo1647 ceo1647 requested changes

@mkhomutov mkhomutov Awaiting requested review from mkhomutov

Requested changes must be addressed to merge this pull request.

Assignees

@Dmitry-VF Dmitry-VF

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dmitry-VF @ceo1647 @GeertvanHorrik