WIP Integrate Zag.js to improve acessibility #104
Conversation
Accessibility hook
Reviewer's Guide by SourceryThis PR integrates Zag.js to improve accessibility in the UI components, starting with the Collapsible component. The implementation includes setting up Zag.js infrastructure, modifying the existing Collapsible component to use Zag.js, and adding necessary build and initialization tasks. Class diagram for new ZagHook componentclassDiagram
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, isRoot)
}
class ZagHook {
+mounted()
+updated()
+destroyed()
+context()
}
Component <|-- ZagHook
note for Component "Handles component lifecycle and rendering with Zag.js"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
bluzky
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bluzky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please document the breaking changes in this PR, particularly the change of default collapsible state from open to closed. Consider adding migration notes for existing users.
Here's what I looked at during the review
- 🟡 General issues: 2 issues 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.
|
|
||
| continue? = Mix.shell().yes?("Do you want to continue with the installation?") | ||
|
|
||
| if continue?, do: :ok, else: {:error, "Installation aborted"} |
There was a problem hiding this comment.
issue: Error handling should clean up any partial installation when user aborts
Consider adding cleanup logic to remove any files or changes made before the abort
| this.component = new Component(this.el, this.context()); | ||
| this.component.init(); | ||
| } catch (error) { | ||
| console.error("Error mounting component:", error); |
There was a problem hiding this comment.
issue (bug_risk): Error in mounted() should prevent further execution to avoid inconsistent state
Consider re-throwing the error or implementing proper error recovery to maintain component consistency
priv/static/assets/zag/ZagHook.js
Outdated
| import * as componentsModules from "./index"; | ||
| import { camelize, getBooleanOption, getOption, normalizeProps } from "./utils"; | ||
|
|
||
| class Component { |
There was a problem hiding this comment.
issue (complexity): Consider extracting prop and event handling logic into dedicated PropManager and EventManager classes to better organize the Component class.
The Component class could be simplified by extracting prop and event management into separate classes. This maintains coordination while reducing complexity. Consider:
class PropManager {
constructor(el) {
this.el = el;
this.prevAttrsMap = new WeakMap();
}
applyProps(attrs, isRoot = false) {
const oldAttrs = this.prevAttrsMap.get(this.el) || {};
Object.entries(attrs).forEach(([key, value]) => {
if (!key.startsWith('on')) {
this.applyAttribute(key, value, oldAttrs[key], isRoot);
}
});
this.prevAttrsMap.set(this.el, attrs);
}
}
class EventManager {
constructor(el) {
this.el = el;
this.handlers = new Map();
}
bindEvents(attrs) {
Object.entries(attrs)
.filter(([key]) => key.startsWith('on'))
.forEach(([key, handler]) => {
const eventName = key.substring(2).toLowerCase();
this.bindEvent(eventName, handler);
});
}
cleanup() {
this.handlers.forEach((handler, event) => {
this.el.removeEventListener(event, handler);
});
this.handlers.clear();
}
}Then simplify Component to use these:
class Component {
constructor(el, context) {
this.el = el;
this.context = context;
this.propManager = new PropManager(el);
this.eventManager = new EventManager(el);
}
spreadProps(el, attrs, isRoot = false) {
this.propManager.applyProps(attrs, isRoot);
this.eventManager.bindEvents(attrs);
return () => this.eventManager.cleanup();
}
}This separation:
- Makes the code more testable and maintainable
- Reduces cognitive load by separating concerns
- Keeps coordination in the Component class
- Makes error handling clearer with focused responsibilities
priv/static/assets/zag/ZagHook.js
Outdated
| } | ||
|
|
||
| renderItem(item) { | ||
| const value = item.dataset.value; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const value = item.dataset.value; | |
| const {value} = item.dataset; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
priv/static/assets/zag/ZagHook.js
Outdated
| const apply = (attrName) => { | ||
| // avoid replacing id on root element because LiveView | ||
| // will lose track of it and DOM patching will not work as expected | ||
| if (attrName === "id" && isRoot) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (attrName === "id" && isRoot) return; | |
| if (attrName === "id" && isRoot) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
priv/static/assets/zag/ZagHook.js
Outdated
| let value = attrs[attrName]; | ||
| const oldValue = oldAttrs[attrName]; | ||
|
|
||
| if (value === oldValue) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (value === oldValue) return; | |
| if (value === oldValue) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
priv/static/assets/zag/ZagHook.js
Outdated
|
|
||
| if (value === oldValue) return; | ||
|
|
||
| if (typeof value === "boolean") value = value || undefined; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (typeof value === "boolean") value = value || undefined; | |
| if (typeof value === "boolean") { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
priv/static/assets/zag/ZagHook.js
Outdated
| if (key.startsWith("on")) setup(key); | ||
| else apply(key); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (key.startsWith("on")) setup(key); | |
| else apply(key); | |
| if (key.startsWith("on")) { | |
| setup(key); | |
| } else apply(key); | |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| export const normalizeProps = createNormalizer((props) => { | ||
| return Object.entries(props).reduce((acc, [key, value]) => { | ||
| if (value === undefined) return acc; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (value === undefined) return acc; | |
| if (value === undefined) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| export const toStyleString = (style) => { | ||
| return Object.entries(style).reduce((styleString, [key, value]) => { | ||
| if (value === null || value === undefined) return styleString; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (value === null || value === undefined) return styleString; | |
| if (value === null || value === undefined) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
WIP Implement hook for select
add files related to zag
|
@bluzky, we might need to consider adding support for Zag's collection API, rather than handling collections as an option. This would make the integration easier for components like |
|
Thanks, @selenil, It's great if we can define a better way to handle collection. It's a bit hacky building collection when init Zag component. |
|
@bluzky I will take:
Also, we could now implement two new components using Zag's foundation: I will first work on supporting collections in the hook, and then I will move on to the components. We can remove the following components from the list, as they aren't supported by Zag:
These components are either too simple or don't have significant accessibility requirements (with a few exceptions). In my view, these components are essentially complete, needing only improvements in documentation and/or minor accessibility attributes. The |
|
@selenil that's great, the list is now much shorter. I've updated JS code which I fix some issues when testing on Storybook |
|
@bluzky the hook handles all interactivity, but it won't work properly if the component uses classes like We could also provide a system to react to certain events in the component with custom code. About the syntax, we could use a map or a list of tuples. def render(assigns) do
~H"""
<.some_component listeners={%{on_open_change: [:client, :server], on_value_change: [:client]}}></.some_component>
"""
end
def render(assigns) do
~H"""
<.some_component listeners={[{:on_value_change, :client, :server}, {:on_value_change, :client}]}></.some_component>
"""
endLet me know what you think about this. |
Improve parts handling
|
Thanks @selenil, I've just come back from my holiday. |
The component's interaction logic is handled by Zag; we don't need extra js code. Beyond that, Zag also allows to execute custom js code when certain events occur. For example, in the Users can listen for these events using This event system is entirely optional – the component works perfectly fine without any custom code. This is to offer users the flexibility to extend the component's functionality and respond to specific interactions as needed. I'll open a PR in the next days implementing it. |
|
@selenil yeah I got your idea. |
Implement event listeners system
Improve tabs
Improve slider
Integrating ZagJS for dropdown menu
Support bind zag api values to elements
…ext-in-slider Add support for output text in slider
Improve progress
|
@bluzky we can remove the |
Updated component
Some components are so simple so I don't think they need Zagjs dependencies
currently, it works fine