Create CLiveRegion component
#206
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| * Indicates what types of changes should be presented to the user. | ||
| * @default "all" | ||
| */ | ||
| "aria-relevant"?: |
There was a problem hiding this comment.
In the React package, there is the React.AriaAttributes type signature that is used to retrieve the types for the aria-relevant and aria-atomic attributes, instead of hard coding them here.
Is their a Vue version of this signature??
There was a problem hiding this comment.
Ah I see. I don't think there is such a type in Vue.js. all of them usually fall under the HTMLAttributes type exported from Vue.
There was a problem hiding this comment.
Whoops! Forgot about that. 😅
TylerAPfledderer
added
type: feature or enhancement ⚡️
| export class LiveRegion { | ||
| region: HTMLElement | null | ||
| options: Required<LiveRegionOptions> | ||
| parentNode: HTMLElement | ||
|
|
||
| constructor(options?: LiveRegionOptions) { | ||
| this.options = getOptions(options) as any | ||
| this.region = getRegion(this.options) | ||
| this.parentNode = this.options.parentNode | ||
| if (this.region) { | ||
| this.parentNode.appendChild(this.region) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Message provided to the region to be read out by the Screen Reader. | ||
| * | ||
| * Message can be supplied on trigger of some event (i.e. button click) | ||
| */ | ||
| public speak(message: string) { | ||
| this.clear() | ||
| if (this.region) { | ||
| this.region.innerText = message | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Removes the region. | ||
| */ | ||
| public destroy() { | ||
| if (this.region) { | ||
| this.region.parentNode?.removeChild(this.region) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Clears the inner text of the region | ||
| */ | ||
| public clear() { | ||
| if (this.region) { | ||
| this.region.innerText = "" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can't this class be a component instead? It seem that it only creates/appends attributes to certain elements, which seems better as a component to me.
Also, using methods such as removeChild should only be done when there is no other solution, otherwise it's a bit of an anti-pattern in Vue. So i'd say that if we have the choice let's create a component.
| function setup(region: HTMLElement, options: Required<LiveRegionOptions>) { | ||
| region.id = options.id || "chakra-live-region" | ||
| region.className = "__chakra-live-region" | ||
| region.setAttribute("aria-live", options["aria-live"]) | ||
| region.setAttribute("role", options.role) | ||
| region.setAttribute("aria-relevant", options["aria-relevant"]) | ||
| region.setAttribute("aria-atomic", String(options["aria-atomic"])) | ||
| Object.assign(region.style, { | ||
| border: "0px", | ||
| clip: "rect(0px, 0px, 0px, 0px)", | ||
| height: "1px", | ||
| width: "1px", | ||
| margin: "-1px", | ||
| padding: "0px", | ||
| overflow: "hidden", | ||
| whiteSpace: "nowrap", | ||
| position: "absolute", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Seems better to me to pass a Ref instead of an HTMLElement might be better for reactivity.
Also, this setup function also looks like it can also just be a component
| export function useLiveRegion(options?: LiveRegionOptions) { | ||
| const liveRegion = reactive(() => new LiveRegion(options)) | ||
|
|
||
| watchEffect((cleanup) => cleanup(() => liveRegion().destroy())) | ||
|
|
||
| return liveRegion() | ||
| } |
There was a problem hiding this comment.
After thinking about it, seem better to have a CLiveRegion component instead of a hook imo.
All the options passed can basically be props and the rest of the code would be much simpler and more aligned with the Vue philosophy.
It will also allow to remove some of the code. I think that eventhough we need to be as close to the React package, there are some things that need to be done differently due to the different philosophy between React and Vue.
What is your opinion about this @codebender828 , @TylerAPfledderer ?
There was a problem hiding this comment.
@Shyrro @codebender828 for context and my understanding of LiveRegion, it is a class because the hook triggers an instance which renders an element out of view of the visual user; a hidden portal of sorts specific to the use of a acreen reader. Then you pull methods (events) from the instance through the hook, to apply to what triggers are used in a given component.
The developer can certainly access the other data to make their own reason, but it did not seem to be intended by default for someone to have to make an element just for a screen reader to readout a simple message on change from some event.
I would not mind converting to a component, with composables to house the props that can be exported separately, but then the question would be how this new component is to be implemented in the environment.
I'm primarily thinking about how the speak method is currently executed versus an alternative approach.
There was a problem hiding this comment.
Tbh if you want to access methods of the components or other data you can expose them through the expose method of your component. They'll then be available through template refs
And for the fact the the component is outside the screen it doesn't really matter if you wrap it in a Teleport or CPortal
It just feels the "Vue way" so to say. Also, when manipulating the DOM directly through document.createElement etc, basically nothing is tracked so you're instance doesn't have any reactivity whatsoever.
I might be wrong about this since this is a special hook, but it just feels more natural in Vue to have in this case a logical component instead of a hook.
There was a problem hiding this comment.
Understood! I'll take a look at expose.
I'll play around with it, because you could probably argue the same case against direct DOM manipulation in React here with the reactivity, so maybe I can ask one of the React folks on the reason for this approach. 🤔
Better for me to know at least so I can get a better idea on differences! 😄
There was a problem hiding this comment.
Let me know if u need any more clarification on what i'm saying or any more explanation on this approach.
We can even discuss it in a call, might be easier
TylerAPfledderer
added
status: work in progress 🐜
TylerAPfledderer
changed the title
useLiveRegion composable and ClassCLiveRegion component
3 participants
Pull request checklist
Please check if your PR fulfills the following requirements:
/start features)
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Closes #195
What is the new behavior?
LiveRegionclass which sets up region, establishes the aria options, and destroys the region on component unmount.Does this introduce a breaking change?
Other information