🎨 Palette: [UX improvement] Enhance accessibility of icon-only buttons in Ollama settings#73
🎨 Palette: [UX improvement] Enhance accessibility of icon-only buttons in Ollama settings#73bobdivx wants to merge 1 commit into
Conversation
…s in Ollama settings Added `aria-label`, `title`, and `aria-hidden="true"` to SVGs for icon-only buttons in `OllamaTab.tsx`. Also added focus ring classes to improve keyboard navigation visibility. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request introduces accessibility improvements for icon-only buttons in the Ollama settings tab, including ARIA labels, focus rings, and hidden SVGs, alongside general code formatting updates. The review feedback highlights several critical issues: the compatibility state type should mark ok as optional to handle untested models, and existing properties like disabledManually must be preserved when updating compatibility state. Additionally, the compatibility badge should verify that comp.ok is defined to avoid false negatives, and proper error handling with res.ok checks should be added to the instance toggle and delete operations.
| const [compatibility, setCompatibility] = useState< | ||
| Record< | ||
| string, | ||
| { ok: boolean; testedAt?: string; disabledManually?: boolean } |
There was a problem hiding this comment.
The ok property should be optional (ok?: boolean). If a model has never been tested but is manually disabled or enabled, the backend API /api/toggle-ollama-model saves the configuration without the ok field. This results in a compatibility object that lacks the ok property, violating the strict ok: boolean type.
| { ok: boolean; testedAt?: string; disabledManually?: boolean } | |
| { ok?: boolean; testedAt?: string; disabledManually?: boolean } |
| setCompatibility((prev) => ({ | ||
| ...prev, | ||
| [model]: { ok: data.ok, testedAt: new Date().toISOString() } | ||
| [model]: { ok: data.ok, testedAt: new Date().toISOString() }, |
There was a problem hiding this comment.
When updating the compatibility state after a test, we should preserve the existing properties of the model's compatibility state (such as disabledManually). Otherwise, running a test on a manually disabled model will temporarily clear its disabled status in the UI until the page is reloaded.
| [model]: { ok: data.ok, testedAt: new Date().toISOString() }, | |
| [model]: { ...prev[model], ok: data.ok, testedAt: new Date().toISOString() }, |
| {isDisabled && ( | ||
| <span class="px-1.5 py-0.5 rounded bg-gray-200 text-gray-500 text-[9px] font-bold uppercase">Désactivé</span> | ||
| )} | ||
| {comp && !isDisabled && ( |
There was a problem hiding this comment.
Since a model can be manually toggled without ever being tested, comp might exist but not have an ok status. If comp.ok is undefined, the badge will render as "Forge KO" because comp.ok evaluates to falsy. We should ensure comp.ok is defined before rendering the compatibility badge.
| {comp && !isDisabled && ( | |
| {comp && comp.ok !== undefined && !isDisabled && ( |
| await fetch("/api/ollama-instances", { | ||
| method: "PUT", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| id: instance.id, | ||
| enabled: instance.enabled ? 0 : 1, | ||
| }), | ||
| }); | ||
| load(); |
There was a problem hiding this comment.
Similar to other API calls, we should check res.ok when toggling an instance to ensure any server-side errors are caught and displayed to the user.
const res = await fetch("/api/ollama-instances", {
method: "PUT",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
id: instance.id,
enabled: instance.enabled ? 0 : 1,
}),
});
if (!res.ok) throw new Error("Échec de la modification de l'état");
load();
| await fetch(`/api/ollama-instances?id=${id}`, { method: "DELETE" }); | ||
| load(); |
There was a problem hiding this comment.
The fetch API does not throw an error on non-ok HTTP status codes (e.g., 400 or 500). We should check res.ok and throw an error if the deletion fails, so that the error is caught and displayed to the user via setError.
const res = await fetch("/api/ollama-instances?id=" + id, { method: "DELETE" });
if (!res.ok) throw new Error("Échec de la suppression");
load();
💡 What: Added ARIA labels, standard focus-visible rings (
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[#175B37]), andaria-hidden="true"to inner SVGs for icon-only action buttons (Test, Enable/Disable, Edit, Delete) in the Ollama settings tab.🎯 Why: To improve the accessibility of the interface for screen readers, as icon-only buttons lacked context. Keyboard navigation is also improved with clear visual focus rings.
📸 Before/After: Visual changes add a green focus ring when navigating via keyboard.
♿ Accessibility: Ensures that screen readers correctly identify button purposes rather than reading out meaningless SVG paths.
PR created automatically by Jules for task 4868165403177010367 started by @bobdivx