Accessibility hook #87
Conversation
Reviewer's Guide by SourceryThis PR implements a hook to integrate the Zag accessibility library with SaladUI components. The implementation includes modifications to Mix tasks for component installation and initialization, along with the addition of a new ZagHook JavaScript module that handles the integration between Zag's state machines and the components. Class diagram for the new ZagHook integrationclassDiagram
class Component {
- el
- context
- service
- api
- cleanupFunctions
+ init()
+ initializeComponent()
+ destroy()
+ cleanup()
+ parts(el)
+ initService(component, context)
+ initApi(component)
+ render()
+ renderPart(root, name, api, opts)
+ renderItem(item)
+ spreadProps(el, attrs)
}
class ZagHook {
+ mounted()
+ destroyed()
+ context()
}
Component --> ZagHook
note for Component "Handles the integration of Zag state machines with components"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @selenil - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider making the automatic npm package installation optional or add version constraints to prevent potential dependency conflicts.
- Error messages in ZagHook.js could be more specific to help with debugging, especially in the component initialization and context parsing sections.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
priv/static/assets/zag/ZagHook.js
Outdated
| } | ||
| } | ||
|
|
||
| spreadProps(el, attrs) { |
There was a problem hiding this comment.
issue (complexity): Consider splitting the spreadProps method into smaller, focused methods for handling events and attributes separately
The spreadProps method handles too many concerns, making it hard to maintain and understand. Consider splitting it into focused methods:
spreadProps(el, attrs) {
const eventHandlers = this.setupEventHandlers(el, attrs);
this.updateAttributes(el, attrs);
return () => this.cleanupEventHandlers(eventHandlers);
}
setupEventHandlers(el, attrs) {
const handlers = [];
Object.entries(attrs)
.filter(([key]) => key.startsWith('on'))
.forEach(([key, handler]) => {
const event = key.substring(2).toLowerCase();
el.addEventListener(event, handler);
handlers.push([event, handler]);
});
return handlers;
}
updateAttributes(el, attrs) {
Object.entries(attrs)
.filter(([key]) => !key.startsWith('on'))
.forEach(([key, value]) => {
if (key === 'id') return; // Preserve LiveView ID
if (value == null) {
el.removeAttribute(key.toLowerCase());
return;
}
if (['value', 'checked', 'htmlFor'].includes(key)) {
el[key] = value;
} else {
el.setAttribute(key.toLowerCase(), typeof value === 'boolean' ? value || undefined : value);
}
});
}
cleanupEventHandlers(handlers) {
return () => handlers.forEach(([event, handler]) =>
this.el.removeEventListener(event, handler)
);
}This refactoring:
- Separates event handling from attribute management
- Removes the unnecessary WeakMap
- Simplifies the attribute update logic
- Makes the code more testable and maintainable
The functionality remains identical while reducing cognitive load.
|
@selenil could you please give me an example, it's hard to imagine how it work |
Sure. Check out the collapsible component. This is how we make a component work with the hook. Now, the hook can recognize it and automatically add all the necessary accessibility attributes. Additional options can be configured using the data-options attribute. I will also evaluate the hook's performance to ensure it doesn't become a heavy load when multiple components are on the screen. |
|
Thanks @selenil, I'm quite busy for a few days, I'll come back to this PR next week |
|
Hi @selenil, I saw you import components from |
Hi @bluzky, the |
This PR introduces a hook to integrate the Zag library with SaladUI components, inspired by @cschmatzler's work in Turboprop.
Zag is a framework-agnostic library that provides state machines for building accessible, interactive components. When a component mounts, the hook invokes the appropriate state machine to add accessibility and interactive features.
Zag is not distributed as a single npm package, but rather as a set of individual packages for each component. The
salad.addtask will install the correct package for the component and also import it in anindex.jsfile in user's project. This way, the hook could access to the correct packages for each component dynamically. Thesalad.inittask is also modified to do the neccesary setup in order to use the hook.To make a component works with the hook:
Add
phx-hook="ZagHook"attribute and anidto the component's root element. For components that don't need a manualidwe can use theunique_idhelper to generate a random id.Add
data-component="component-name"to the root element. This should match the Zag component you want to target. The name typically matches the name of the SaladUI component, but it can be different if needed.Add a
data-partsattribute to the component's root element to indicate its structure. For example, in thecollapsiblecomponent, thetriggerandcontentare both parts of the component, so we set:data-parts={Jason.encode!["trigger", "content"]}. For components containing items with their own parts (like accordions), we don't need to include the item parts here.For components that contain other components, like
accordion_item, we follow the same pattern. So, in the accordion item component:data-parts={Jason.encode!(["trigger", "content"])}. Additionally, these types of components require adata-valueattribute. For instace, we could use something like:data-value="accordion-item-1.Add a
data-partattribute to each part of the component. The value should be the name of the part. For example, in the collapsible component, we have:data-parts={Jason.encode!(["trigger", "content"])}, so we need to adddata-part="trigger"anddata-part="content"to thecollapsible_triggerandcollapsible_contentcomponents respectively. Is not necessary to add thedata-part="root"attribute to the root component. For components that are items, likeaccordion_item, we need to adddata-part="item"to identify it as a part with its own parts.(optional) Add a
data-optionsattribute to the component's root element. This is a map with all options that Zag accepts for the component. The keys are the name of the options (in any case, the hook will convert them to camelCase) and the values are the posibles values for that option, as an array. If the option is a boolean, pass:boolinstead. Each option also needs to have a correspondingdata-optionattribute with the value of the option. For example, we could set:data-options={Jason.encode!(%{open: :bool, direction: ["ltr", "rtl"]})} data-open={@open} data-direction={@direction}. All of this should match the Zag api for the component.(optional) Add a
data-listenersattribute to the component's root element. This is a list of events that the component could emit from the client to the server. The hook will add the corresponding listeners to the component. This follows the Zag api for events where each event is called:onEventChange. As the naming is consistent, we only need to provide the event name in thedata-listenersattribute, in lower case, and the hook will find the correct event. For example, in thecollapsiblecomponent, we could set:data-listeners={Jason.encode!(["open"])}to listen to theonOpenChangeevent. It is probably better let the user set the value ofdata-listeners, as any event included in listeners will need to be handled in the live view or an exception will be raised. So, instead we should use:data-listeners={Jason.encode!(@listeners)}.(optionally) The hook will manage the interaction logic, so JS commands are not required in the component, but they remain functionals. We could keep them as public functions to allows users to programatically interact with the component.
Summary by Sourcery
Integrate the Zag library with SaladUI components by introducing a new hook that utilizes state machines for accessibility and interactivity. Update the
salad.addandsalad.inittasks to support this integration by installing necessary packages and setting up configurations.New Features:
Enhancements:
salad.addtask to install the correct Zag package for each component and import it into anindex.jsfile.salad.inittask to set up the necessary files and configurations for using the Zag integration hook.