-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add aria-options for layout components #133
Conversation
That looks pretty good to me; I do think adding some aria attributes in as props seems like a good compromise between giving people control over attributes and the potential performance issues you were describing. My one suggestion would be to add a prop for My experience with <canvas>Some fallback text</canvas> as "Some fallback text." So it might make sense to put the title slot inside the I definitely think there shouldn't be defaults (other than |
For the SVGs, the slot there works. Or I have limited experience with With the HTML one, I'm not sure about the use case for the title slot there. For my use cases, I would use the |
Thanks you both!
I was thinking of doing the
Nope I have no use case in mind! Was just putting it in there to be consistent with the others. |
I do think there are a few cases where you'd want to have a slot instead of a prop for the
My instinct is that it would also make sense to have the title operate as a slot for That said, I could see a case for having a <slot name="title">{#if title}{title}{/if}</slot>
I'm also not sure about the use-case for the |
@maxblee that sounds good. for the title fallback, wouldn't you want this instead <slot name="title">{#if title}<title>{title}</title>{/if}</slot> |
Okay I think these are all done. I added |
Yeah. Because canvas fallback text can support plain HTML, I think it probably makes sense to have a fallback slot for <canvas {...attrs}>{fallbackText}</canvas> you'd have something like <canvas {...attrs}><slot name="fallback">{#if fallbackText}{fallbackText}{/if}</slot></canvas> Otherwise this looks spot on. Thanks! |
For the svg components, it defaults to putting the text in a |
I don't think so. From what I've read <canvas>Some description</canvas> is fine. I think the benefit to having the |
Very good that makes sense. I'll work on the docs for this. I think I'll add an accessibility section in addition to adding information on props and slots for each of the layout components |
One more change to put it. I'm told that the aria-* elements on the HTML div are rather meaningless and it should have a |
Thanks for everyone's help on this! |
@mhkeller Thank you for being so responsive and thoughtful about this! |
Thanks for all the feedback! Let me know how it goes and if it needs adjustment! Glad we got this figured out for now. |
Fixes #71 and replaces #74.
Adds accessibility attributes. @maxblee and @patriciatiffany how does this look? I'm not sure if this is the best way to handle the title. I'm picturing the usage like this:
Is
title
also used for canvas in the same way or does it need to be different? Should there be defaults?