Polish source onboarding hierarchy - #20
Conversation
There was a problem hiding this comment.
Code Review
This pull request reorganizes the workspace sources section by moving the navigation component and implementing a CSS Grid-based layout with named areas for better structure and accessibility. Documentation was also added to define the onboarding hierarchy. Review feedback suggests synchronizing rail widths across stylesheets for consistency, utilizing theme variables instead of hardcoded colors, and removing redundant CSS properties from non-grid containers.
|
|
||
| .source-follow-layout { | ||
| gap: var(--space-4); | ||
| grid-template-columns: minmax(0, 1fr) minmax(230px, 270px); |
There was a problem hiding this comment.
The rail width defined here (minmax(230px, 270px)) is inconsistent with the definition in apps/web/app/styles/sources.css (minmax(220px, 260px)). Unless this 10px difference is intentional for the premium layout, these values should be synchronized to ensure a consistent user experience across different layout modes. Consider using a shared CSS variable for the rail width.
| grid-area: main; | ||
| gap: var(--space-4); | ||
| padding: clamp(var(--space-4), 1.4vw, var(--space-6)); | ||
| border-color: rgba(31, 95, 189, 0.16); |
There was a problem hiding this comment.
This hardcoded RGBA value uses the same base color as the --accent variable (#1f5fbd). To improve maintainability and ensure theme consistency, it is better to use the variable with a color-mix function.
| border-color: rgba(31, 95, 189, 0.16); | |
| border-color: color-mix(in srgb, var(--accent), transparent 84%); |
| .source-follow-aside { | ||
| grid-area: aside; | ||
| grid-template-columns: 1fr; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb81835a64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .source-follow-aside { | ||
| grid-area: aside; | ||
| grid-template-columns: 1fr; |
There was a problem hiding this comment.
Reset aside column span for new sources grid
The new two-column mapping sets .source-follow-aside to grid-area: aside, but it never clears the existing responsive override in apps/web/app/styles/responsive.css (@media (max-width: 1360px) sets grid-column: 1 / -1). As a result, at widths between 1181px and 1360px the aside still spans both columns, so the status rail drops below the main/nav area instead of staying in the right rail intended by this change. Add an explicit reset (for example grid-column: auto) in the new layout rules to avoid inheriting the legacy span behavior.
Useful? React with 👍 / 👎.
Summary
/sourcesonboarding so the primary add-source form is first in DOM and visual hierarchy.docs/ui-system.md.Evidence
npm run buildpassed.npm run test:unitpassed.RSSMASTER_PLAYWRIGHT_MODULE=C:\Users\user\Desktop\RSSmaster\output\playwright-runtime\node_modules\playwright npm run check:sourcespassed after final CSS changes.RSSMASTER_PLAYWRIGHT_MODULE=C:\Users\user\Desktop\RSSmaster\output\playwright-runtime\node_modules\playwright npm run check:layoutpassed after final CSS changes.output/playwright/page-audit-sources-desktop-1180.png,output/playwright/page-audit-sources-tablet-1024.png,output/playwright/page-audit-sources-mobile-390.png.UX Scorecard
Closes #8