fix(tile): change expandable tile to button #5609
Conversation
ghost
requested review from
|
Deploy preview for carbon-elements ready! Built with commit 27366a0 |
|
Deploy preview for carbon-components-react ready! Built with commit 27366a0 https://deploy-preview-5609--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit ac44e74 |
|
Deploy preview for carbon-components-react ready! Built with commit ac44e74 https://deploy-preview-5609--carbon-components-react.netlify.com |
|
Since the chevron is a button, it is still receiving focus. Going to try and figure out a different pattern to address this problem since this isn't the right solution. |
|
Wondering why we need the icon to be wrapped in a button anyways 🤔Since the expansion is always triggered by clicking the tile itself, it seems like it may be unnecessary markup. Wonder if we can just switch it to a div, and move the |
| {...other} | ||
| onClick={this.handleClick} | ||
| onKeyPress={this.handleKeyDown} | ||
| // onKeyPress={this.handleKeyDown} |
There was a problem hiding this comment.
Did you mean to remove this line, or keep it? Or is this PR still WIP...?
There was a problem hiding this comment.
This is still a WIP but since I changed the outer div to a button, handleKeydown is no longer necessary since handleClick will be triggered for keydown events. Definitely meant to remove it completely though! Not just comment it out, my bad!
|
@tw15egan agreed! I'm thinking of going that route! Unfortunately, it seems like screenreaders don't read the expanded state unless it's on |
| @@ -284,23 +284,20 @@ describe('Tile', () => { | |||
| expect(wrapper.state().expanded).toEqual(false); | |||
| }); | |||
|
|
|||
| it('displays the default tooltip for the button depending on state', () => { | |||
| fit('displays the default tooltip for the button depending on state', () => { | |||
There was a problem hiding this comment.
Did you want to revert this line?
|
The chevron is not receiving focus but the tile also isn't getting the focus border either. Is that intended? |
|
@aagonzales definitely not intentional! Where are you seeing this? I wasn't able to reproduce it |
|
oh that's weird I just checked it again and now its there. I swear I didn't see it earlier. Oh well seems to be correct now 😆 |
| @@ -22861,6 +22861,14 @@ Tile styles | |||
| } | |||
| } | |||
|
|
|||
| .#{$prefix}--tile--expandable { | |||
| width: inherit; | |||
There was a problem hiding this comment.
Honestly I think there is not many use cases for width: inherit. So may I ask what this is for?
There was a problem hiding this comment.
I just wanted it to get the width that was already defined in bx--tile, but I see that doing width: 100% gets the same result so I'll change it to that!
abbeyhrt
changed the title
Closes #5557
This PR adds a FF specific override for the border and focus it was supplying to the Chevron in ExpandableTile.
Testing / Reviewing
Go to the ExpandableTile example in Storybook on Firefox and make sure that the Chevron does not receive focus.