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

Open / close main panel with swipes #156

Merged
merged 8 commits into from
Aug 9, 2024

Conversation

baileys-li
Copy link
Contributor

@baileys-li baileys-li commented Aug 8, 2024

Fix #26

Minor changes for main panel logic

view/main/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
view/main/logo.svg Outdated Show resolved Hide resolved
@baileys-li baileys-li requested a review from ai August 8, 2024 18:23
main.main
button.main_expand( aria-label="Expand or collapse panel" type="button" )
main.main.layout_main#main-panel
button.main_expand( aria-label="Main panel" type="button" aria-expanded='true' aria-controls="main-panel" )
Copy link
Member

Choose a reason for hiding this comment

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

Why we change label? The new label doesn't describe the action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aria-expanded already say if element is collapsed or expanded. Screen reader even notify user on attribute change.

So with old label it sounds like oiled oil (yeah, I didn't found English idiom)

PS It's supposed to notify only that the status has changed, but on my mac, Voice Over is re-reading the entire message:

Main panel, expanded, button
Main panel, collapsed, button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will ask in a11y chat how people deal with such cases

Copy link
Member

@ai ai Aug 8, 2024

Choose a reason for hiding this comment

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

Main panel, expanded, button
Main panel, collapsed, button

Good note about expanded/collapsed. But I still prefer verb there.

Maybe Toggle panel?

Copy link
Member

Choose a reason for hiding this comment

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

Also, let’s remove main since this word doesn’t say anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to toggle.

PS Miss @glafirazhur confirmed that it's ok when a button in such contexts doesn't have a action label. For example, accordion pattern

@ai
Copy link
Member

ai commented Aug 8, 2024

I will be able to review it a little later. I need to see how it actually work.

@baileys-li
Copy link
Contributor Author

Sure

@ai
Copy link
Member

ai commented Aug 8, 2024

Looks great on real device. I am ready for merge when we will fix the label.

@baileys-li baileys-li requested a review from ai August 9, 2024 05:27
@ai ai merged commit d8ed5ae into evilmartians:main Aug 9, 2024
2 checks passed
@ai
Copy link
Member

ai commented Aug 9, 2024

Thanks. Amazing work.

@baileys-li
Copy link
Contributor Author

Thanks

@baileys-li baileys-li deleted the rework/main-panel branch August 9, 2024 05:47
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.

Add main panel gestures on mobile
2 participants