[RFC] [WIP] Watermark Enhancements #2898

amkuchta · 2017-03-17T15:16:02Z

What changed?

I love the functionality of the floating watermark, but don't like that the border stretches, as do any buttons contained within the control. My solution is to implement a watermark that floats outside of the control. Improvements include:

Creation of a DependencyProperty, FloatingWatermarkLocation, which can be used on the following controls:
- TextBox
- ComboBox
- DatePicker
- DateTimePicker
- PasswordBox
- NumericUpDown
Creation of an enum named FloatingWatermarkLocations to outline values acceptable for the newly created dependency property: Interior and Exterior
Updates to the view to demonstrate the new feature (TextBox, DatePicker, DateTimePicker, ComboBox)
Implements Controls:TextBoxHelper.FloatingWatermarkOverrideText, which allows users to set a custom text for floating watermarks if they feel that the current watermark is too long

TODO (Help Needed)

Status on all controls:

TextBox - Fully implemented
PasswordBox - Fully implemented
DatePicker - Fully implemented
DateTimePicker - Fully implemented
ComboBox - Partially implemented: the FloatingWatermarkLocation works well on non-editable ComboBox controls... but if editing is enabled, forget about it (it works on initial load, but I cannot get it to work after the text has been changed). I could use some help wiring this together
NumericUpDown - No implementation: The Dependency Property can be used for the control, but for some reason, the TemplateBinding isn't working...

@thoemmi , I figured that you may be the best person to take a look, since you did the initial creation of the UseFloatingWatermark feature. Thoughts? (Sorry for the duplicate tag - I moved this to its own PR, where it belongs)

Closed Issues

…xterior of the control

amkuchta · 2017-03-18T00:46:31Z

@punker can I ask why the "Breaking" label? I took care to ensure that "Interior" is the default setting - users don't have to declare it, and their code will work fine following the update, as demonstrated in the MetroDemo in my branch (unless you think something in ComboBox or NumericUpDown will break existing functionality)

thoemmi · 2017-03-19T17:00:42Z

Reviewing this PR is hard as it also reformats many XAML files. @amkuchta please consider using XAML Styler, a VS extension for formatting XAMl files. @punker76 added a settings file for XAML Styler.

amkuchta · 2017-03-19T17:17:29Z

@thoemmi no problem, I'll pull the settings file. I already use XAML Style for my other projects, but my formatting for them is different. I'll ingest the settings for MA.M and add another commit later today (US time)

…f Watermark TextTrimming

amkuchta · 2017-03-19T20:26:41Z

@thoemmi @punker76 formatting is updated per the referenced XAML Styler settings file - should be easier to follow along with now. I also began implementing Controls:TextBoxHelper.WatermarkTrimming (#2884), but for some reason TemplateBinding Controls:TextBoxHelper.WatermarkTrimming is not overriding the default style on PART_Message. It is the only DependencyProperty that is not working.

This also implements #2889 by applying TextWrapping={TemplateBinding TextWrapping} on PART_Message for TextBox controls - I still need to add this to the other controls.

punker76 · 2017-03-19T20:29:20Z

I must really say make changes always without any code formatting changes together, cause this makes it always hard to read and review the changes... so maybe the settings file is not up to date :-( oh man

amkuchta · 2017-03-19T20:31:42Z

@punker76 noted. Might not be a bad idea to add text about the XAML Style settings file in the PR template - it would probably help to make sure that everyone is on the same page when they submit

punker76 · 2017-03-19T20:32:52Z

@amkuchta will do this, sorry

amkuchta · 2017-03-19T20:35:50Z

@punker76 no need to apologize - just trying to help keep guys like me from creating headaches for guys like you

…ng - now it works!

amkuchta · 2017-03-19T22:03:06Z

Controls:TextBoxHelper.WatermarkTrimming now works - I realized that creating my own enum was the issue, I need to use the provided System.Windows.TextTrimming.

I'll incorporate this throughout (including on floating watermarks) tonight / tomorrow

michaelmairegger · 2017-03-20T07:27:01Z

src/MahApps.Metro/MahApps.Metro.Shared/Controls/Helper/TextBoxHelper.cs

@@ -31,6 +31,8 @@ public class SpellCheckSeparator : Separator, ISpellCheckMenuItem
    {
    }

+    public enum FloatingWatermarkLocations { Interior, Exterior };


Could you consider renaming this enum to singular. Because plural enum name should be used for enums with a [FlagAttribute]

michaelmairegger · 2017-03-20T07:27:06Z

src/MahApps.Metro/MahApps.Metro.Shared/Controls/Helper/TextBoxHelper.cs

+        [AttachedPropertyBrowsableForType(typeof(NumericUpDown))]
+        [AttachedPropertyBrowsableForType(typeof(DatePicker))]
+        [AttachedPropertyBrowsableForType(typeof(TimePickerBase))]
+        public static string GetFloatingWatermarkLocation(DependencyObject obj)


Why returning a string when FloatingWatermarkLocation is of type FloatingWatermarkLocations?

michaelmairegger · 2017-03-20T07:27:09Z

src/MahApps.Metro/MahApps.Metro.Shared/Controls/Helper/TextBoxHelper.cs

+        [AttachedPropertyBrowsableForType(typeof(DatePicker))]
+        [AttachedPropertyBrowsableForType(typeof(TimePickerBase))]
+        [AttachedPropertyBrowsableForType(typeof(NumericUpDown))]
+        public static string GetWatermarkTrimming(DependencyObject obj)


Why returning a string when WatermarkTrimming is of type TextTrimming?

@xxMUROxx I will rename the enum, and the two functions return string because FloatingWatermarkLocation started out as typeof string and I forgot to change it 😆 I'll change this too - good catch!

…and GetFloatingWatermarkLocation now return the correct types; #2884 and #2889 fully implemented

amkuchta · 2017-03-20T18:38:17Z

@xxMUROxx updated to address your concerns with the latest commit

…erty

amkuchta · 2017-03-20T21:42:56Z

Thought of another item to implement that is Watermark based: an ability to override longer watermarks with something shorter for display when FloatingWatermarks are in use (I need to change it to a MultiDataTrigger that also takes into account UseFloatingWatermarks, but this works for a demo purpose). Notes:

If FloatingWatermarkOverrideText is set, Floating watermark becomes the set text
If FloatingWatermarkOverrideText is not set, the value provided for Watermark is used for the floating values (same as before)
This is not a breaking change

Example GIF:

amkuchta · 2017-03-20T23:04:10Z

As a side note, if you all (looking at you @thoemmi and @punker76) until after I have finished implementing things & stuff, I can let you know. I should only have one commit left, though (excluding any changes you guys come across)

…for override text to be MultiDataBinding that takes UseFloatingWatermark into account

amkuchta · 2017-03-21T12:20:37Z

@punker76 @thoemmi I'm done! I updated the main text for the PR to reflect what is done and what still needs to happen - short version is that editable ComboBox controls and NumericupDown still are not working with the FloatingWatermarkLocation. I have updated the views to show some implementation, and once these are approved, I will work them into my proposed changes for the MetroDemo.

Let me know if you guys want me to change anything, and thanks in advance for the help with the ComboBox and NumericUpDown issues!

amkuchta · 2017-04-20T15:03:19Z

@punker76 @thoemmi I see now that there are some conflicts with this PR and the develop branch now that v1.5.0 is out, and I know that you guys were having some issues with completing the reviews because of the code formatting. Would it make things easier on you guys if I closed this PR, moved the changes to a newer copy of develop, and resubmitted? I don't particularly want to have to redo the work, but if it helps you guys out (which, in turn, helps me by getting the changes approved faster), I am willing to do it.

Thoughts?

michaelmairegger · 2017-04-20T15:18:38Z

You have two options: - merge develop into this branch (easiest) - rebase the branch on develop (you have to force push) I personally prefer the first as you can see on open PR created by me Any changes you make to this branch (even rewriting history) is reflected to this PR. So no need to close and create a new one

amkuchta · 2017-04-20T15:29:51Z

@xxMUROxx, the issue is that the history itself is messed up - changes that I made were overwritten by XAML formatting updates, which makes analyzing the diff hard for @punker76 and @thoemmi. Creating a new pull request (which has the correct formatting applied from the start) will make analyzing the diff easier, which speeds up the review process.

thoemmi

I don't understand why you need FloatingWatermarkOverrideText. It's not that you toggle between interior and exterior watermark at runtime und need two different texts.

BTW, a new PR would be appreciated 😉

amkuchta · 2017-04-20T18:47:28Z

@thoemmi FloatingWatermarkOverrideText is a separate feature from FloatingWatermarkLocation - the two can be used independently. Consider the following example:

I have a watermark that reads "Email Address (you@example.com)". I want the example in the watermark, I only want my floating watermark to read "Email Address" (without the example, because it looks ugly). To achieve this, I set FloatingWatermarkOverrideText="Email Address". Now, I have the example in the TextBox, and the "pretty" version floating, when the example is no longer required because the TextBox is populated.

I could then go a step further by deciding to have the watermark float exterior to the box by setting FloatingWatermarkLocation="Exterior", though this is not necessary if I am happy with having the floating watermark inside of the TextBox control.

Hope that clarifies!

Also, I'll work the new PR tonight 😄

thoemmi · 2017-04-20T19:25:07Z

Ok, I see your point. However, personally I don't like watermark's appearing and disappearing, regardless if inside or outside the textbox: it changes the layout, and the user might be irritated by the moving controls. Like I said in #1825, what you're talking about is a header and not a watermark.

amkuchta · 2017-04-20T19:33:03Z

@thoemmi the watermark shouldn't be "appearing or disappearing" - the transitions are the same, it's really just a matter of where the watermark is being shown and what it says.

To address the "header" topic, wouldn't the current floating watermark really be just a header that is inside of the border of the TextBox? And as far as the moving control goes, the FloatingWatermark already shifts layout when it expands the TextBox - this is no different, just that the TextBox itself is not expanding in size, simply shifting downward to make room for the new watermark / header.

In any case, both points are semi-moot; the changes I have made use your original settings as the standard, and users must explicitly declare my options in order for them to take effect. If the users like yours better, they do not have to adjust their implementation at all, and current users will not see any change in how the controls function following the incorporation of this feature.

thoemmi · 2017-04-20T19:53:22Z

Indeed, I'm not a fan of the already existing FloatingWatermark, so my personal opinion is not only about your PR 😉

I'm curious how many users of MahApps.Metro are really using FloatingWatermark. I'd guess it's only a very small fraction. The MetroTextBox style is getting out of hand, and the maintainability will suffer from every additional "exotic" feature. I'm sure your request is reasonable in your scenario, but I'm afraid that someday we'll have a "feature explosion". IMHO if a requirement is too special, you better use a custom style in your own application instead of passing it on to Mahapps.Metro (or any other generic library), which must serve lots of different use cases.

Anyway, that's just my $ 0.02. At the end of the day @punker76 has the final say.

amkuchta · 2017-04-21T12:53:28Z

@thoemmi it's good to know that I shouldn't take it personally! In all honesty, I understand the "feature creep" that you are concerned about - unmanageable code is a pain. I wanted to implement this at the MahApps level for two reasons:

Others may find it useful or may like my style just a bit better, which could up the "adoption" of the floating watermark
(And more importantly) it was easier for me to make the changes at the library level as opposed to a custom implementation, as all of the code is in place for transitions, etc.

Like most devs, I'm lazy 🤣

amkuchta · 2017-04-25T02:09:00Z

Closing to create (another) new PR that is cleaner and has the correct formatting... It'll be a bit, largely because git sucks.

Updates to provide the option to move the floating watermark to the e…

abfdcb7

…xterior of the control

punker76 added the Breaking Change label Mar 17, 2017

punker76 requested review from thoemmi and punker76 March 17, 2017 15:24

amkuchta mentioned this pull request Mar 17, 2017

[Feature Request] Watermark Trimming #2884

Closed

Reformatting XAML because Jan asked so nicely

a364ff4

Formatting changes (per XAML Styler Settings); begin implementation o…

0ffb36f

…f Watermark TextTrimming

amkuchta changed the title ~~[RFC] [WIP] Exterior Floating Watermark~~ [RFC] [WIP] Watermark Enhancements Mar 19, 2017

Changes Controls:Watermark.WatermarkTrimming to be of type TextTrimmi…

dc20c51

…ng - now it works!

michaelmairegger suggested changes Mar 20, 2017

View reviewed changes

FloatingWatermarkLocation enum is now singular; GetWatermarkTrimming …

dffc772

…and GetFloatingWatermarkLocation now return the correct types; #2884 and #2889 fully implemented

Begins implementation of FloatingWatermarkOverrideText DependencyProp…

8664165

…erty

punker76 added Feature Request and removed Breaking Change labels Mar 21, 2017

amkuchta added 2 commits March 21, 2017 08:05

Fully Implements FloatingWatermarkOverrideText; Corrects DataBinding …

fedbf0b

…for override text to be MultiDataBinding that takes UseFloatingWatermark into account

Updates views to show implementation examples

1bc11ac

Merge branch 'develop' into watermark-enhancement

e63bd9e

amkuchta and others added 2 commits March 23, 2017 07:21

Merge branch 'develop' into watermark-enhancement

95a6330

Merge branch 'develop' into watermark-enhancement

9c74b21

thoemmi requested changes Apr 20, 2017

View reviewed changes

amkuchta closed this Apr 25, 2017

amkuchta mentioned this pull request Apr 29, 2017

Watermark Enhancements #2948

Closed

4 tasks

punker76 added Feature and removed Feature Request labels Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] [WIP] Watermark Enhancements #2898

[RFC] [WIP] Watermark Enhancements #2898

amkuchta commented Mar 17, 2017 •

edited

Loading

amkuchta commented Mar 18, 2017

thoemmi commented Mar 19, 2017

amkuchta commented Mar 19, 2017

amkuchta commented Mar 19, 2017

punker76 commented Mar 19, 2017 •

edited

Loading

amkuchta commented Mar 19, 2017

punker76 commented Mar 19, 2017

amkuchta commented Mar 19, 2017

amkuchta commented Mar 19, 2017

michaelmairegger Mar 20, 2017

michaelmairegger Mar 20, 2017

michaelmairegger Mar 20, 2017

amkuchta Mar 20, 2017

michaelmairegger Mar 20, 2017

amkuchta commented Mar 20, 2017

amkuchta commented Mar 20, 2017

amkuchta commented Mar 20, 2017

amkuchta commented Mar 21, 2017

amkuchta commented Apr 20, 2017

michaelmairegger commented Apr 20, 2017 via email •

edited

Loading

amkuchta commented Apr 20, 2017

thoemmi left a comment

amkuchta commented Apr 20, 2017 •

edited

Loading

thoemmi commented Apr 20, 2017

amkuchta commented Apr 20, 2017

thoemmi commented Apr 20, 2017 •

edited

Loading

amkuchta commented Apr 21, 2017

amkuchta commented Apr 25, 2017

[RFC] [WIP] Watermark Enhancements #2898

[RFC] [WIP] Watermark Enhancements #2898

Conversation

amkuchta commented Mar 17, 2017 • edited Loading

What changed?

TODO (Help Needed)

Closed Issues

amkuchta commented Mar 18, 2017

thoemmi commented Mar 19, 2017

amkuchta commented Mar 19, 2017

amkuchta commented Mar 19, 2017

punker76 commented Mar 19, 2017 • edited Loading

amkuchta commented Mar 19, 2017

punker76 commented Mar 19, 2017

amkuchta commented Mar 19, 2017

amkuchta commented Mar 19, 2017

michaelmairegger Mar 20, 2017

Choose a reason for hiding this comment

michaelmairegger Mar 20, 2017

Choose a reason for hiding this comment

michaelmairegger Mar 20, 2017

Choose a reason for hiding this comment

amkuchta Mar 20, 2017

Choose a reason for hiding this comment

michaelmairegger Mar 20, 2017

Choose a reason for hiding this comment

amkuchta commented Mar 20, 2017

amkuchta commented Mar 20, 2017

amkuchta commented Mar 20, 2017

amkuchta commented Mar 21, 2017

amkuchta commented Apr 20, 2017

michaelmairegger commented Apr 20, 2017 via email • edited Loading

amkuchta commented Apr 20, 2017

thoemmi left a comment

Choose a reason for hiding this comment

amkuchta commented Apr 20, 2017 • edited Loading

thoemmi commented Apr 20, 2017

amkuchta commented Apr 20, 2017

thoemmi commented Apr 20, 2017 • edited Loading

amkuchta commented Apr 21, 2017

amkuchta commented Apr 25, 2017

amkuchta commented Mar 17, 2017 •

edited

Loading

punker76 commented Mar 19, 2017 •

edited

Loading

michaelmairegger commented Apr 20, 2017 via email •

edited

Loading

amkuchta commented Apr 20, 2017 •

edited

Loading

thoemmi commented Apr 20, 2017 •

edited

Loading