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

[Refacto-557] Button: Add UIControl #588

Merged
merged 8 commits into from
Nov 22, 2023
Merged

Conversation

robergro
Copy link
Contributor

@robergro robergro commented Oct 30, 2023

  • Add UIControl on Button
  • Deprecate old var, func and init
  • Add setImage, setTitle, setAttributed methods
  • Override isSelected, isEnabled and isHighlighted var
  • Update the demo
  • Update the Wiki

Copy link
Contributor

@michael-zimmermann michael-zimmermann left a comment

Choose a reason for hiding this comment

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

Two things I saw in the demo app:

  • When the state is just icon, the trailing spacing seems to be bigger than the leading spacing
  • When the state of the highlighted content is just text, the icon is still shown. This is the same for disabled and selected.
    Simulator Screenshot - iPhone 15 Pro - 2023-10-30 at 13 34 58
    Simulator Screenshot - iPhone 15 Pro - 2023-10-30 at 13 34 30

@robergro
Copy link
Contributor Author

Ooo Thank you @michael-zimmermann ,
I sent a new commit.

It not solve everything but it's better.
Currently, the visibility of the label and icon is manage by the view model but it's very hard to manage all state with the viewModel too I think.

So now, we can have a problem if the button has just an icon (on normal mode) and image + text (on highlighted mode) because the size of the button is a square.

This is why i need to split the component to:

  • Button
  • IconButton (always square, withtout title)
  • Toggle Button

And I hope with that, all our problems will be solved.

Copy link
Contributor

@LouisBorleeAdevinta LouisBorleeAdevinta left a comment

Choose a reason for hiding this comment

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

I'll need a bit more testing, to continue :)

@robergro robergro force-pushed the refacto/button/integration branch from ba5f371 to 096dc80 Compare November 7, 2023 09:02
@robergro robergro linked an issue Nov 21, 2023 that may be closed by this pull request
@robergro robergro merged commit ebca448 into main Nov 22, 2023
4 checks passed
@robergro robergro deleted the refacto/button/integration branch November 22, 2023 10:27
jacklyn-situmorang pushed a commit that referenced this pull request Nov 27, 2023
…ion"

This reverts commit ebca448, reversing
changes made to 49a41ae.
jacklyn-situmorang pushed a commit that referenced this pull request Nov 27, 2023
Revert "Merge pull request #588 from adevinta/refacto/button/integrat…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Button] Refactoring: Add common Control management
3 participants