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

Entry ClearButtonVisibility Handler #564

Merged
merged 10 commits into from
Mar 24, 2021
Merged

Entry ClearButtonVisibility Handler #564

merged 10 commits into from
Mar 24, 2021

Conversation

bkaankose
Copy link
Contributor

@bkaankose bkaankose commented Mar 21, 2021

Description of Change

This PR implements ClearButtonVisibility property for iOS and Android.

Implements #379

Additions made

  • Adds ClearButtonVisibility property to IEntry.
  • Adds MapClearButtonVisibility method to EntryHandler.
  • Adds IOnTouchListener and IOnFocusChangeListener to handle clear button's visibility in Android handler when the focus is changed and button is touched.
  • Adds override for custom clear button drawable in Android handler.

Notes

  • I needed to implement touch and focus listener to handle button's visibility in Android handler. This implementation might need to be reworked later on when a views or controls have general touch and focus listening mechanism (if they the team has plans to do so).

  • Device tests are missing (WIP) but functionality work as expected for both iOS and Android.

  • I don't know if this overriding approach in the handler for custom clear button drawable is correct. Open for suggestions.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)
  • Targets a single property for a single control (or intertwined few properties)
  • Adds the property to the appropriate interface
  • Avoids any changes not essential to the handler property
  • Adds the mapping to the PropertyMapper in the handler
  • Adds the mapping method to the Android, iOS, and Standard aspects of the handler
  • Implements the actual property updates (usually in extension methods in the Platform section of Core)
  • Tags ported renderer methods with [PortHandler]
  • Adds an example of the property to the sample project (MainPage)
  • Adds the property to the stub class
  • Implements basic property tests in DeviceTests

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Looks really clean and nice. Besides the tests you said you are still working on, there was one question more than a change.

Comment on lines +27 to +29
// Returns the default 'X' char drawable in the AppCompatEditText.
protected virtual Drawable GetClearButtonDrawable()
=> ContextCompat.GetDrawable(Context, Resource.Drawable.abc_ic_clear_material);
Copy link
Member

Choose a reason for hiding this comment

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

@PureWeen Just pointing this out. Not sure what the official way to override things are like this. Should it be some mapper or some other registration that can be injected without requiring an override in a new type? We obviously need to be careful with having a bajillion mapper functions for everything. Maybe we need to clarify where the line of mapper/override/parameter is.

src/Core/src/Handlers/Entry/EntryHandler.Android.cs Outdated Show resolved Hide resolved
@bkaankose
Copy link
Contributor Author

Device tests are implemented for both platforms.

@hartez hartez merged commit 14fbdce into dotnet:main Mar 24, 2021
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jul 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants