Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Refactor MessageActions using ARIA Toolbar pattern #4799

Closed
wants to merge 4 commits into from

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Jun 19, 2020

This patch includes two commits to help reviewers distinguish between code reorganization and new implementation.

I haven't been able to test the alternate layout for "call in progress" because I don't have any contacts yet.

Signed-off-by: Mike Pennisi [email protected]

@t3chguy
Copy link
Member

t3chguy commented Jun 20, 2020

This is intended to resolve element-hq/element-web#11316

That issue is about the per-Message Actions Bar which has React, Reply, Edit, More Options
The buttons this changed were more like the Composer Actions. I'm not entirely sure what the best ARIA pattern for them is, toolbar doesn't seem insane but also keeping them as distinct buttons isn't terrible

@jugglinmike
Copy link
Contributor Author

Got it. I've submitted gh-4827 to hopefully provide the desired fix. For this patch, is there anyone we should contact to help make a decision?

@t3chguy
Copy link
Member

t3chguy commented Jun 25, 2020

I would suggest chatting with the AT users in https://matrix.to/#/#a11y:riot.ovh as to what they'd find most obvious

@jugglinmike
Copy link
Contributor Author

Oh, that's great! I'll follow up there

@jryans
Copy link
Collaborator

jryans commented Jun 25, 2020

@jugglinmike Does #4827 replace this one, or are they both meant to be reviewed...?

@jugglinmike
Copy link
Contributor Author

Hi @jryans! This patch is independent, so it's still fit for review. That said, since it wasn't what was requested in element-hq/element-web#11316, so @t3chguy and I have been talking with folks in the Matrix room mentioned above.

At least one person's on board, though to be honest, I don't know the criteria for moving forward with this patch.

@t3chguy
Copy link
Member

t3chguy commented Jul 5, 2020

Can you do a DCO sign-off in the PR description as one of your commits lacks one, thanks :)

Copy link
Member

@t3chguy t3chguy 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 generally, small changes required

Comment on lines 220 to 228
<UploadButton key="upload"
tabIndex={tabIndex('upload')}
roomId={props.room.roomId} />,
<EmojiButton key="emoji"
tabIndex={tabIndex('emoji')}
addEmoji={addEmoji} />,
<Stickerpicker key="sticker"
tabIndex={tabIndex('sticker')}
room={props.room} />,
Copy link
Member

Choose a reason for hiding this comment

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

our multi-line props style is to do 4 space indent on the line after the component opener

Comment on lines 240 to 245
<CallButton key="call"
tabIndex={tabIndex('call')}
roomId={props.room.roomId} />,
<VideoCallButton key="videocall"
tabIndex={tabIndex('videocall')}
roomId={props.room.roomId} />,
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Comment on lines 234 to 236
<HangupButton key="hangup"
tabIndex={tabIndex('hangup')}
roomId={props.room.roomId} />,
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@jugglinmike
Copy link
Contributor Author

Can you do a DCO sign-off in the PR description as one of your commits lacks one, thanks :)

Sure, although I don't intend for this branch to be merged as a series of distinct commits. Let me know if you would rather thatI squash them down on my end.

@jryans
Copy link
Collaborator

jryans commented Mar 4, 2021

Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed. If you are still interested in pursuing this feature, please discuss with us in #element-dev:matrix.org to find a good approach forward.

@jryans jryans closed this Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants