-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fixes accessibilityLabel bug on ASButtonNode that has no title #1573
Fixes accessibilityLabel bug on ASButtonNode that has no title #1573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@maicki Can you take a look at this one when you have time? |
I think we should change the logic a bit, the if clause is really hard to read. And I also think we don't need to call the setter not before within the if clause:
Also @SterlingWaves would you mind adding this
@nguyenhuy Thoughts? |
Yeah, agree with your suggestions, @maicki. |
@maicki @nguyenhuy updated. |
Thanks @SterlingPinterest. I assume @maicki would approve this PR so I'm gonna go ahead and merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes seems fine.
We use the
ASButtonNode
as a pure icon button that has no title, and I also give this button an accessibility label. Since this buttton'stitleNode
will be non-nil if theself.titleNode
getter is ever called, my accessibility label will get reset to empty string every time the button's state changes (selected
,enabled
, etc.). However, I'd like my button's accessibility label to remain consistent in this case. This PR addresses this problem, i.e. if the button title and the new title are both either empty or nil, we do not reset the accessibility label. Also added corresponding test.