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

[Button] Refactoring #557

Closed
5 of 6 tasks
robergro opened this issue Oct 20, 2023 · 7 comments · Fixed by #573, #579, #701 or #774
Closed
5 of 6 tasks

[Button] Refactoring #557

robergro opened this issue Oct 20, 2023 · 7 comments · Fixed by #573, #579, #701 or #774
Assignees
Labels
Enhancement New feature or request UIKit Component developed for iOS UIKit

Comments

@robergro
Copy link
Contributor

robergro commented Oct 20, 2023

  • Rename Text to title to have same property as a native UIButton (PR)
  • Inherite from UIControl and not UIView (PR)
  • Add title and attributedTitle for state (get and set func) (PR)
  • Split the Button to have a IconButton (PR)
  • Update snapshot testing (PR)
  • Update Wiki
@robergro robergro self-assigned this Oct 20, 2023
@robergro robergro added Enhancement New feature or request ✅ Ready All the needed informations are detailed within the ticket to include it in another sprint UIKit Component developed for iOS UIKit labels Oct 20, 2023
@robergro robergro added this to the Q4 - Sprint 2 milestone Oct 20, 2023
@robergro robergro modified the milestones: Q4 - Sprint 2, Q4 - Sprint 3 Oct 30, 2023
@robergro robergro modified the milestones: Q4 - Sprint 3, Q4 - Sprint 4 Nov 10, 2023
@robergro robergro modified the milestones: Q4 - Sprint 4, Q4 - Sprint 5 Nov 27, 2023
@thomas-leguellec thomas-leguellec removed the ✅ Ready All the needed informations are detailed within the ticket to include it in another sprint label Nov 28, 2023
@robergro robergro modified the milestones: Q4 - Sprint 5, Q4 - Sprint 6 Dec 12, 2023
@robergro robergro linked a pull request Dec 22, 2023 that will close this issue
7 tasks
@elisa-hery
Copy link

Test branch : refacto/button/all

@elisa-hery
Copy link

ℹ️ what changed ?

  • now the content can be updated according to the state (eg : a content for default state, a different one for selected state)
  • we now have a isSelected state

@elisa-hery
Copy link

elisa-hery commented Jan 19, 2024

@robergro here are 2 questions about button :

  • question : what happens if a text is too big for the button ? Is there an ellipsis in the middle of the text, like for a large dynamic types ? We need to check if design is ok with that
Capture d’écran 2024-01-19 à 15 23 57

- a11y : contrary to iconButton, voice over recognize the component but does not identify it as a button, and I think it's not normal. `Traits` should not be defined as "static text", WDYT ?

Capture d’écran 2024-01-19 à 15 28 27

@elisa-hery
Copy link

@robergro questions about iconButton :

  • about small size : according to the specs, touch area should be 44px and it's not.
Enregistrement.de.l.ecran.2024-01-19.a.12.08.00.mov
  • Also, i'm not sure if we should have small square iconButtons (but it's no big deal)
isolated
  • accessibility : voice over canot read this button even when it's enabled, I think it's a problem
Enregistrement.de.l.ecran.2024-01-19.a.12.59.35.mov
  • please note that functionality is hard to test because demo app is not so close from real behavior of iconButton. It would be great to have a beta-tester Polaris team, to see how it works in real conditions

@robergro robergro linked a pull request Jan 24, 2024 that will close this issue
@robergro
Copy link
Contributor Author

robergro commented Jan 24, 2024

  • question : what happens if a text is too big for the button ? Is there an ellipsis in the middle of the text, like for a large dynamic types ? We need to check if design is ok with that
    We don’t have other option, if the available space is not big enough, the text is truncated … 
It is the same thing for all component with a static height

  • a11y : contrary to iconButton, voice over recognize the component but does not identify it as a button, and I think it's not normal. Traits should not be defined as "static text", WDYT ?
    I fixed it, the consumer need to set a property to have the expected content when voice over is enabled. On the demo I setted a value ("My icon button")

  • about small size : according to the specs, touch area should be 44px and it's not.
    The size is 32px and on iOS, it is a very bad practice to update the size of the tapeable area (here ton 44px), we already informed all the team about this choice/decision.

  • Also, i'm not sure if we should have small square iconButtons (but it's no big deal)
    Agree with that, but I think we can change this rule later, the UI/UX should just be careful if they really want to use this size…

  • accessibility : voice over canot read this button even when it's enabled, I think it's a problem
    You’re right. It should be ok now ! I added a property on the demo app to switch between the default value and the value which can be set by the consumer (if he want to have an another value)

  • please note that functionality is hard to test because demo app is not so close from real behavior of iconButton. It would be great to have a beta-tester Polaris team, to see how it works in real conditions
    Yes you’re right ! I added

    • on UIKit, on the controlType configuration line a new entry: toggle. With this entry, you can now check a toggle action
    • on SwiftUI a checkbox to activated the toggle mode

@elisa-hery
Copy link

elisa-hery commented Jan 26, 2024

  • A11y is now fixed
Capture d’écran 2024-01-26 à 11 47 53
  • the rest is ok ✅

--> ready for design review !
new branch is : refacto/button/fix

@elisa-hery
Copy link

Approved by @JeremiasUX ✅ (see slack thread here). I'm moving it to RFP !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment