Skip to content
This repository has been archived by the owner on Nov 12, 2020. It is now read-only.

Entry's error text displayed even if there is no ErrorText set #28

Closed
toverux opened this issue Oct 17, 2017 · 12 comments
Closed

Entry's error text displayed even if there is no ErrorText set #28

toverux opened this issue Oct 17, 2017 · 12 comments

Comments

@toverux
Copy link

toverux commented Oct 17, 2017

Bug (Feature Request maybe?)

  • Version Number of Control: 1.0.4
  • Device Tested On: Android 6 API 23
  • Simulator Tested On: Default Android Emulator

Expected Behavior

When ErrorText is empty/null, then the container that contains the error text disappears.

Actual Behavior

The space used for the ErrorText is always there and that eats a LOT of space. I don't know if that's intentional, if it is expected, is it feasible to add a new property like ShowEmptyErrorBlock?

image

Steps to reproduce the Behavior

<Frame>
    <StackLayout Spacing="0">
        <Entry Placeholder="Entry 1" />
        <Entry Placeholder="Entry 2" />
    </StackLayout>
</Frame>

<Frame>
    <StackLayout Spacing="0">
        <xfx:XfxEntry Placeholder="Entry 1" />
        <xfx:XfxEntry Placeholder="Entry 2" />
    </StackLayout>
</Frame>
@ChaseFlorell
Copy link
Contributor

Currently this is by design. It's because if you set the error text and currently don't use up the space, then the view will "jump" when the new view is shown. I felt that was less desirable than having the extra space.

@toverux
Copy link
Author

toverux commented Oct 24, 2017

But that's still a lot of space 😄
I think that the "jump" effect is not problematic if the validation messages are shown when the user clicks the submit/save/whatever button. I agree, maybe that's not the best we can imagine, but I'm pretty sure we can do better than a giant blank space.

Or maybe XfxControls' API consumer will have its own validation system that don't relies on ErrorText.
Personally, I won't need ErrorText on all of my forms (so a new property to hide the block would be cool).

(Verry sorry for being a little insistent, that package is great, but I can't use it because of that and #27. Also, since I'm a bit new to this ecosystem, I don't really know how to fork the project and fix it by myself, testing it in an app, etc).

@ChaseFlorell
Copy link
Contributor

so the layout on Android is the default TextInputLayout from the Material design, and that's the margin/padding that comes with it.

Eventually I want to give the user the option of how to display errors (Default or ToolTip), but I'm not sure when that'll be (#6 is my priority right now). If you use "ToolTip", it won't take up the extra space.

I can take a look at #27 to see if there's anything I can do there in the near future... shouldn't be too hard.

@toverux
Copy link
Author

toverux commented Oct 24, 2017

Yes, no problem, thanks for the entry input :)

@RudolfVonKrugstein
Copy link

How would you pass the option ToolTip or underline error? As an enum?

I would like to contribute but would prefer your input on that so I can do it right.

@ChaseFlorell
Copy link
Contributor

ChaseFlorell commented Oct 31, 2017

Yes, an Enum would be preferable.

        /// <summary>
        /// Gets or Sets whether or not the Error Style is 'Underline' or 'Tooltip'
        /// </summary>
        public ErrorDisplay ErrorDisplay {get;set;} 

and then

public enum ErrorDisplay 
{
    Underline,
    Tooltip
}

If you make it bindable, there's a lot of work to do to change it, so at this point I'm recommending it NOT be bindable.

One thing to keep in mind is that iOS doesn't have a mechanism for this at all, so you'll have to add the whole works by hand. Android already has the feature built into the EditText.

https://developer.android.com/reference/android/widget/TextView.html#setError(java.lang.CharSequence)

@RudolfVonKrugstein
Copy link

Mmmh, ok. Implementing a Tooltip on iOS is some work. But for now I added a pull request, that should address the original problem in this issue. And it will allow extending by tooltip in the future.

So I added the enum:

public enum ErrorDisplay 
{
    Underline,
    None
}

Allowing to remove any Error display.

@ChaseFlorell
Copy link
Contributor

This is fixed in #33, Thanks @RudolfVonKrugstein.

Do you guys think we should cut a patch release with these fixes even though #6 and #25 are outstanding? They're pretty high priority.

@RudolfVonKrugstein
Copy link

I would love a release, but that is just my egocentric view. Now it has all the features I want.

@brux88
Copy link

brux88 commented Nov 30, 2017

hi,great work, its possible a release with this fix?

@ChaseFlorell
Copy link
Contributor

Just following up, V1.1.1 has been released

/cc @RudolfVonKrugstein, @brux88, @toverux

@brux88
Copy link

brux88 commented Dec 13, 2017

thank you very much

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants