Skip to content

Conversation

@afercia
Copy link
Contributor

@afercia afercia commented May 3, 2017

Fixes #628

'is-active': control.isActive
} ) } />
} ) }
aria-pressed={ control.isActive ? 'true' : 'false' }
Copy link
Member

Choose a reason for hiding this comment

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

I think this should just be { control.isActive }.

@afercia
Copy link
Contributor Author

afercia commented May 4, 2017

Now that I look back at that, I think the original intent was to have true and false as strings, because HTML attributes are strings. See also the aria-expanded for the inserter toggle. Minor point though.

@nylen
Copy link
Member

nylen commented May 4, 2017

React should handle this for us. It's worth checking what it's actually doing in the dom though. Is the absence of an aria-pressed attribute the same as false?

@afercia
Copy link
Contributor Author

afercia commented May 4, 2017

Is the absence of an aria-pressed attribute the same as false?

No because the presence of the attribute (even when false) make screen readers announce the button as toggle button instead of just button.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

aria-pressed

@nylen
Copy link
Member

nylen commented May 4, 2017

Thanks for checking, @aduth, I made those last few comments while mobile.

It would be good to circle back to this later and add tests to make sure we're rendering the attribute appropriately (see #641 (comment)).

For now, +1 for merging.

@jasmussen
Copy link
Contributor

I see approval here, so I'm merging.

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.

5 participants