Skip to content

Feat/docs#109

Open
giuliana-gladeye wants to merge 34 commits into
mainfrom
feat/docs
Open

Feat/docs#109
giuliana-gladeye wants to merge 34 commits into
mainfrom
feat/docs

Conversation

@giuliana-gladeye

Copy link
Copy Markdown
Collaborator
  • Added proper documentation to all components using create-documentation skill.
  • Added new styling to docs table
  • Added FUTURE_IMPROVEMENTS.md, a list of component improvements surfaced while writing documentation.
  • Removed Drawer from docs only since component is unfinished. See note in FUTURE_IMPROVEMENTS.md.

@giuliana-gladeye

Copy link
Copy Markdown
Collaborator Author

Note that feat/small-fixes PR #113 holds fixes for this items on FUTURE_IMPROVEMENTS.md:

  • Search Bar — the clear button uses name="XIcon". Lucide's canonical icon is X (XIcon is an alias that exists only in newer versions). If the installed lucide version lacks the alias, IconButton warns and renders null, so the clear button silently disappears. Prefer name="X". packages/ui/src/components/search-bar/search-bar.tsx
  • Checkbox — stray trailing space / {' '} inside the indicator markup. Harmless, minor cleanup. packages/ui/src/components/checkbox/checkbox.tsx
  • Icon — an unknown Lucide string name throws (Icon "" not found), crashing the render, whereas Icon Button with an unknown name only warns and returns null. Align the two: either both throw or both degrade gracefully. packages/ui/src/components/icon/icon.tsx · packages/ui/src/components/icon-button/icon-button.tsx

Comment thread FUTURE_IMPROVEMENTS.md
`packages/ui/src/components/spinner/spinner.tsx`
- **Loading Overlay** — no `role="status"`/`aria-live`, no focus trap, and no `aria-busy` on the blocked content. Screen reader users may not be told loading started, and keyboard focus can still reach content behind the overlay. Add a live region and focus management.
`packages/ui/src/components/loading-overlay/loading-overlay.tsx`
- **Command** — keyboard navigation works, but there is no visual highlight for the active item. cmdk marks it with `data-selected="true"`, but `.command-item` only styles `:hover`/`:focus`. Add a `data-[selected=true]` rule mirroring the hover colors:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visual highlight Looks fine to me on in-app nav search, selects as expected but may need to check how the tab order works on the command component itself?

Comment thread FUTURE_IMPROVEMENTS.md
`packages/ui/src/components/command/command.module.css`
- **Pagination** — numbered page buttons convey the current page only via color (`primary` vs `tertiary`), with no `aria-current="page"`; the control group isn't wrapped in a `<nav aria-label="Pagination">` landmark.
`packages/ui/src/components/pagination/pagination.tsx`
- **Icon Button** — `label` (which becomes `aria-label`) is optional, but an icon-only button with no label is unlabeled for screen readers. Consider making `label` required.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional to prevent double labels when used with tooltips

Comment thread FUTURE_IMPROVEMENTS.md
`packages/ui/src/components/pagination/pagination.tsx`
- **Icon Button** — `label` (which becomes `aria-label`) is optional, but an icon-only button with no label is unlabeled for screen readers. Consider making `label` required.
`packages/ui/src/components/icon-button/icon-button.tsx`
- **Copy Button** — on success the label swaps to "Copied!" but there's no `aria-live` region, so the confirmation isn't announced. Add a visually-hidden live region.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tooltip that appears on copy that says "Copied!"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants