Search TextBlock horizontal alignment centered

[aosyatnik]

No description provided.

[lukechurch]

@Benglin for review please

[Benglin]

Hi @aosyatnik, thanks for the pull request, I hope it familiarizes yourself with GitHub, which is a good start. While the code changes worked well as the first cut, we need to change a little more things here to match the design as outlined in the specification:

Here are my suggestions:

1. The Magnifying Glass icon is not moving with the Search text, it should be. What you can do is to move the Image down before Search text, and put these two together is a horizontally oriented StackPanel.
2. Since the Magnifying Glass and Search are no longer direct child elements of Grid, remove their Grid attributes, move the attribute to StackPanel you just created in plugins nodes #1.
3. The Grid element that contains the Search text now has 4 columns, it needs to be 3.
4. Update the Magnifying Glass to use the same NonEmptyStringToCollapsedConverter binding to hide itself when the search text box is not empty.

That way you can align both the Magnifying Glass and Search text horizontally in the middle. Please let me know if there's anything I can make clearer.

[aosyatnik]

Hi, Ben.
Thanks for your reply. No, everything is clear, but I would rather use
one more Grid, as well as it has ColumnDefinition. I'll send my project
as soon as possible.
Best regards,
Anna.
On 7/16/2014 3:01 AM, Ben Goh wrote:

Hi @aosyatnik https://github.com/aosyatnik, thanks for the pull
request, I hope it familiarizes yourself with GitHub, which is a good
start. While the code changes worked well as the first cut, we need to
change a little more things here to match the design as outlined in
the specification
https://docs.google.com/document/d/1sPlXdh2vsxU7gVbW-iKm1s1JtGYNh9hnvbD8L7wiAww/edit:

Here are my suggestions:

1. The |Magnifying Glass| icon is not moving with the |Search| text,
   it should be. What you can do is to move the |Image| down before
   |Search| text, and put these two together is a horizontally
   oriented |StackPanel|.
2. Since the |Magnifying Glass| and |Search| are no longer direct
   child elements of |Grid|, remove their |Grid| attributes, move the
   attribute to |StackPanel| you just created in plugins nodes #1
   plugins nodes #1.
3. The |Grid| element that contains the |Search| text now has 4
   columns, it needs to be 3.

That way you can align both the |Magnifying Glass| and |Search| text
horizontally in the middle. Please let me know if there's anything I
can make clearer.

—
Reply to this email directly or view it on GitHub
#1954 (comment).

[Benglin]

No worries, @aosyatnik, in my opinion Grid is much heavier than StackPanel due to its extra functionality to allow grid behaviors. So I'd still recommend the use of a StackPanel for this simple case of putting the Magnifying Glass next to Search text. Does that make sense?

[Benglin]

Hi @aosyatnik, I have tried this out and spoken to @elayabharath, this is the desired behaviour:

• In a normal mode, the Search Box is as shown -- both Icon and Search are in the middle
• When user hovers over it, the Icon gets highlighted but nothing moves (existing behaviour)
• When user clicks on the Search box, both Icon and Search jumps to align to the left edge
• When user starts typing, the Search disappears
• When user clears the Search Box and left it empty, the Search reappears

As much as possible we should do this within XAML with the help of Storyboard and Converter to help with the desired behaviour, we should not have to write these in C# (other than of course, the codes in converter). If these additional logic makes the Search Box too complicated, we should factor this out into a separate custom control (so the XAML file won't be overly cluttered).

Please let me know if there's anything that's not clear and I'll clarify.

[Benglin]

This converter is used in various places, please don't change its original intent. We need a different solution.

[aosyatnik]

In the last Vladimir's commit changes about actions was removed.
.
Ben, please review my last commit.

I have tried to use Focus of TextBox as Binding element, but there is one problem: if we have some text in textbox and than lose focus, our icon appears at the center. That's why using focus for binding is wrong solution.

What I propose is to create new property in ViewModel. I called it CanAlingmentToLeft, that would depend not only on Focus, but also on value of textbox. Hope that you will accept this solution.

[Benglin]

This is now SearchIconAlignment property that is bound to StackPanel.HorizontalAlignment as described in my earlier comment regarding the updated XAML. It should return System.Windows.HorizontalAlignment in this case:

public System.Windows.HorizontalAlignment SearchIconAlignment { get { ... } set { ... } };

[Benglin]

Hi @aosyatnik, thanks for the updates. After getting your codes and tweaked it for a while, I think I have found a better way to do that (instead of using GridLength which can be quite error-prone when we change the size of things on the UI).

My suggestion:

Here is the XAML for the search part that we need for this change:

<Grid Name="SearchTextBoxGrid">
    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="*"/>
        <ColumnDefinition Width="Auto"/>
    </Grid.ColumnDefinitions>
    <StackPanel Name="SearchIconAndTextBlockPanel"
                Grid.ColumnSpan="2"
                Width="Auto">
        <StackPanel.HorizontalAlignment>
            <Binding Path="SearchIconAlignment" ... />
        </StackPanel.HorizontalAlignment>
        <Image x:Name="SearchIcon" ... />
        <TextBlock ... />
    </StackPanel>

    <TextBox Name="SearchTextBox" ... />
    <Button Name="SearchCancelButton"
            Grid.Column="1" />
</Grid>

Few things that you notice here:

1. There are only two columns in the grid.
2. SearchIconAndTextBlockPanel has a width set to Auto.
3. SearchIconAndTextBlockPanel spans across 2 grid columns.
4. StackPanel.HorizontalAlignment is now bound to SearchIconAlignment property.
5. SearchCancelButton is now on column 1 instead of column 2.

Whenever focus changes or text changes in SearchTextBox, we will set the values of SearchIconAlignment, thus raising a property changed event. This way the horizontal alignment is updated. Note that since we are returning System.Windows.HorizontalAlignment, we might not need a converter for StackPanel.HorizontalAlignment :)

Please try it out and see if things are working as expected.

Also, please remove unnecessary attributes from these XAML tags, something like Margin or HorizontalAlignment can always make things not work because if the StackPanel is horizontally stretched out to fill the entire grid control, then alignment to the Left or Center will not make any difference.

[Benglin]

One other thing, could you format the XAML part that you are working on? Each attribute should be on its own line (except the first attribute). Here's how:

1. Go to Tools > Options > Text Editor > XAML > Formatting > Spacing
2. Select Position each attribute on a separate line
3. Select Position first attribute on same line as start tag
4. Go to your XAML file
5. Select the parts that you are changing (i.e. the entire Border)
6. Press CTRL + E + F to format it

You can repeat steps #5 and #6 each time you change XAML so that they are always formatted properly. Thanks!

WARNING: Do not change other parts that you are not working on, that will result in too much diff for code review.

[Benglin]

Please leave the statement after if or else condition to a new line, so it doesn't get too long.

[Benglin]

Hi @aosyatnik, thanks for the changes, they look great now! Anyway, I can't merge your changes, you need to pull in new updates. Also since you're going to need another commit, you can incorporate the changes I request above too.

[aosyatnik]

As well as source code was changed, I have added all changes to my project. Now it's exact copy as Sitrus + my changes about SearchTextBox. I hope there won't be any problems about merging them now.

[Benglin]

Closing this as it is replaced by another pull request.