Skip to content
This repository has been archived by the owner on Aug 25, 2023. It is now read-only.

Feature/checkbox update #15

Merged

Conversation

MLAlexIartsev
Copy link
Contributor

@MLAlexIartsev MLAlexIartsev commented Aug 21, 2018

Se limpió la clase MLCheckBox. Se hizo una unificación inicial de los métodos responsables por las animaciones para disminuir la repetición del código.

Se añadió el método público setEnabled:(BOOL)Animated:(BOOL) a MLBooleanWidget que habilita/deshabilita la interacción del usuario. Se agregó un override en MLCheckBox que además tiñe el checkbox de UIColor.ml_meli_mid_grey manteniéndolo en el estado de seleccionado/no seleccionado.

La función nueva se añadió a pedido de Ayelen Santamaría para Combo Recommendations.

Nuevos estados de CheckBox:

Checked & Disabled:
image
Unchecked & Disabled:
image

Efecto de Cambio:
check

Se puede ver el funcionamiento de los cambios de estado en la aplicación de prueba en New UI -> Widgets -> CheckBox, RadioButton & Switch

@pigounet-meli pigounet-meli self-requested a review August 21, 2018 20:40
Copy link
Contributor

@pigounet-meli pigounet-meli left a comment

Choose a reason for hiding this comment

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

Me falta probar en la testapp, te aviso cuando lo revise.
Acordate de actualizar estas neuvas feature en la wiki: https://github.com/mercadolibre/fury_mobile-ios-ui/wiki/Checkbox

#pragma mark - Public Methods
- (void)setEnabled:(BOOL)enabled Animated:(BOOL)animated
{
if (self.userInteractionEnabled == enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usaría un Bool propio del checkbox para mantener el estado enabled/disabled. Nadie te debería modificar el userInteractionEnabled pero podrían y el estado de la ui quedaría raro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lo que me causa un toque de duda ahí es que si no estoy cambiando el userInteractionEnabled, es posible que la interacción con el componente siga siendo manejada desde BooleanWidget. Lo voy a probar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahí lo probé y parece andar bien. Voy a commitear un cambio con una property BOOL isEnabled

self.userInteractionEnabled = enabled;
if (enabled) {
if (self.isBooleanWidgetOn) {
[self fillCheckBoxExternalFromColor:[UIColor ml_meli_mid_grey] ToColor:[UIColor ml_meli_blue] Animated:animated];
Copy link
Contributor

Choose a reason for hiding this comment

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

Movería los [UIColor ...] a funciones que devuelvan el color para cada estado (checkboxColorOn, checkboxColorOff y checkboxColorDisabled). Así se actualizan desde un solo lugar en caso de querer cambiar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Claro, yo lo dejé así pero en el fondo estaba pensando que estaría bueno que haya algún config de colores en el futuro. Lo voy a armar con dos métodos así está un pasito más cerca a tener los colores configurables.

- (void)setOnBooleanWidgetAnimated:(BOOL)animated
{
if (self.isBooleanWidgetOn) {
if (self.isBooleanWidgetOn || !self.userInteractionEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Acá también usaría un booleano interno no la userInterationEnabled.

[clearAnimation setAnimations:[NSArray arrayWithObjects:colorClearAnimation, opacityClearAnimation, nil]];

[self.checkBoxInternalLayer addAnimation:clearAnimation forKey:@"animateClear"];
if (!self.userInteractionEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mismo criterio, no usaría el userInteractionEnabled como flag.

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #15 into develop will increase coverage by 0.12%.
The diff coverage is 87.14%.

@@             Coverage Diff             @@
##           develop      #15      +/-   ##
===========================================
+ Coverage    69.41%   69.53%   +0.12%     
===========================================
  Files           47       47              
  Lines         3106     3089      -17     
===========================================
- Hits          2156     2148       -8     
+ Misses         950      941       -9

return CGPointMake(x, y);
- (UIColor *)disabledColor
{
return [UIColor ml_meli_light_grey];
Copy link
Contributor

Choose a reason for hiding this comment

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

Los colores desde el PR antes eran ml_meli_mid_grey y ml_meli_grey. Cambiaste por que van estos otros o fue un error al pasarlo a las funciones?

Copy link
Contributor

@pigounet-meli pigounet-meli left a comment

Choose a reason for hiding this comment

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

@MLAlexIartsev El widget deshabilitado esta cambiando de estado igual y cuando lo volvés a habilitar se muestra con el estado cambiado.
Y por otro lado tendríamos que validar la animación del check al pasar de habilitado a deshabilitado y viceversa.


- (void)testEnableCheckBox
{

Copy link
Contributor

Choose a reason for hiding this comment

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

Te quedo este test vacío.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uy, me pasa por dormido. Ahí muevo los checks y ordeno bien

@pigounet-meli pigounet-meli merged commit 5d70572 into mercadolibre:develop Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants