Frontend and UI design implementation#27
Frontend and UI design implementation#27sonofluiz wants to merge 49 commits intoDanielHerr:previewfrom
Conversation
…ts. Closes #10. Paginate app category lists. Closes DanielHerr#19. Add FFOS sideloading instructions. Closes #3.
…lHerr#24. Add theme control. Closes DanielHerr#22. Add site nav. Closes DanielHerr#12.
Frontend style refresh
Frontend style refresh
…rk mode color overrides in the sematic colors file
Frontend style refresh
Frontend style refresh
…ponents, and styled the scroll bar
Frontend style refresh
Frontend style refresh
…onsiveness and functionality with the backend
Added base layout and index page designs. Updated components for resp…
…sher page are unfinished
Styled all static pages. App cards grid, app details pages, and publi…
Finished styling all pages. Search results page is not fully styled
…restructed favicon loading for index and base layout pages
styled pagefind search result elements and restyled the search page. …
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a complete frontend and design system implementation for the Firefox Apps Archive project. It establishes a robust design token system, modernizes the icon library to use Phosphor icons, and updates the project's build and styling infrastructure for better compatibility and maintainability. New UI components and web app manifests are also added to enhance the user experience and PWA capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a comprehensive design token system, updates build processes, and refactors several UI components. Key changes include adding a new DESIGN_TOKENS_README.md and updating README.md with build instructions. The astro.config.mjs was modified to remove concurrency: 1, and build/deploys.txt was updated with new build commands. package.json now includes new dependencies for Phosphor icons, dialog polyfill, and PostCSS plugins, along with an Astro version bump. A postcss.config.cjs was added to transpile CSS custom properties for Firefox OS v1 compatibility. Several CSS files (common.css, details.css, lists.css, search-results.css) received extensive styling updates, and new CSS files (dialog_polyfill.css, nav.css, noscript.css, pagination.css, search.css) were added. JavaScript files (common.js, install.js, install.mjs, pagination.js, search.js) were updated for theme persistence, search functionality, and dialog polyfill. Icon assets were updated, and a detailed ICON_CONVERSION_SESSION_REPORT.md was added. The src/categories.json was updated, and numerous Astro UI components (AppIcon, BadgeNotification, Button, ButtonAlphabetMenuExpansion, ButtonAppTypes, ButtonBack, ButtonCategory, ButtonClose, ButtonColorTheme, ButtonCta, ButtonGalleryNav, ButtonLetter, ButtonMenu, ButtonPageNav, ButtonReadMore, ButtonSearch, ButtonsAlphabetPagination, CardApp, CardAppTypes, ChipHostedStatus, Footer, H1, H2, HeadlineAppType, InputSearchBar, InputText, LabelPageNavButton, LabelSectionNumber, LabelWarning, Link, LinkPublisher, MenuAlphabetPagination, MenuAppCategories, MenuNavSidebar, MenuPageNavButtons, Modal, RadioButton, SectionAppDescription, SectionAppDetail, SectionHeadline, SectionReleaseNotes, SectionScreenGallery, SectionSupport, SectionVersion, TagAppStatus) were created or significantly refactored to align with the new design system and improve functionality. Critical issues were identified regarding the use of the unsupported :has() pseudo-class in Firefox for styling and toggle functionality, which breaks keyboard accessibility and UI consistency in key components like RadioButton, InputSearchBar, and Header. A ReferenceError was noted in public/common.js due to accessing a potentially non-existent DOM element. Missing icon files for dynamically constructed <img> tags in src/components/categories.astro will lead to broken images. The build command in build/deploys.txt uses platform-specific syntax, hindering portability, and contains a redundant npx. Minor issues include an unnecessary align-items property in public/common.css, redundant semicolons, and the use of alert() for error reporting, which degrades user experience. Inconsistent focus styles across button components were also noted.
| .radio-button:has(.radio-input:focus-visible) .border-overlay { | ||
| border-width: var(--border-lg); | ||
| border-color: var(--on-surface-light-faded); | ||
| } | ||
|
|
||
| /* Active state (mouse click) - thick darker border */ | ||
| .radio-button:active .border-overlay { | ||
| border-width: var(--border-lg); | ||
| border-color: var(--on-surface-light-faded); | ||
| } | ||
|
|
||
| /* Selected state - primary background */ | ||
| .radio-button[data-checked="true"] { | ||
| background-color: var(--on-surface-light-primary); | ||
| } | ||
|
|
||
| .radio-button[data-checked="true"] .border-overlay { | ||
| border-color: var(--on-surface-light-primary); | ||
| } | ||
|
|
||
| /* Selected + Active state (mouse click) - just thick border, no focus ring */ | ||
| .radio-button[data-checked="true"]:active .border-overlay { | ||
| border-width: var(--border-lg); | ||
| border-color: var(--on-surface-light-faded); | ||
| } | ||
|
|
||
| /* Selected + Focus state (keyboard only) - show focus ring */ | ||
| .radio-button[data-checked="true"]:has(.radio-input:focus-visible) .border-overlay { | ||
| border-width: var(--border-lg); | ||
| border-color: var(--on-surface-light-faded); | ||
| /* Add visible focus ring around selected button */ | ||
| box-shadow: | ||
| var(--shadow-app-icon), | ||
| 0 0 0 2px var(--ink-05); | ||
| } |
There was a problem hiding this comment.
The :has(.radio-input:focus-visible) selector is used to apply focus styles. This will not work in Firefox, as it does not support the :has() pseudo-class. This breaks keyboard accessibility for this component in a key target browser.
Consider using JavaScript to toggle a focus class on the parent label, or restructuring the HTML to use sibling selectors (+ or ~) if possible.
| .search-bar-inner:hover:not(:has(input:focus)):not([data-disabled="true"]) { | ||
| box-shadow: none; | ||
| } | ||
|
|
||
| /* ======================================== | ||
| STATE: FOCUS | ||
| ======================================== */ | ||
| .search-input:focus-visible { | ||
| outline: none; | ||
| } | ||
|
|
||
| .search-bar-inner:has(input:focus-visible) { | ||
| box-shadow: none; | ||
| } |
There was a problem hiding this comment.
The :has(input:focus) and :has(input:focus-visible) selectors are used here to style the search bar when the input is focused. Since the :has() pseudo-class is not supported in Firefox, this styling will not be applied for Firefox users, leading to an inconsistent UI experience.
A common alternative is to use JavaScript to add a focused class to the parent wrapper (search-bar-inner) when the input receives focus and remove it on blur. You can then style the wrapper based on this class.
| :global(body:has(#nav:popover-open)) .action-buttons :global(.button-menu-popover) { | ||
| display: none; | ||
| } | ||
|
|
||
| :global(body:has(#nav:popover-open)) .action-buttons :global(.button-close-popover) { | ||
| display: inline-flex; | ||
| } | ||
| } | ||
|
|
||
| /* No-JS fallback: checkbox toggle */ | ||
| @supports not selector(:popover-open) { | ||
| :global(body:has(#nav_toggle_input:checked)) .action-buttons :global(.button-menu-label) { | ||
| display: none; | ||
| } | ||
|
|
||
| :global(body:has(#nav_toggle_input:checked)) .action-buttons :global(.button-close-label) { | ||
| display: inline-flex; | ||
| } | ||
| } |
There was a problem hiding this comment.
The :has() pseudo-class is used here to toggle the visibility of the menu and close buttons. However, :has() is not supported in Firefox, which is a key target platform for this project. This will cause the menu toggle functionality to be broken for Firefox users.
To ensure cross-browser compatibility, I recommend using a JavaScript-based approach to toggle a class on a parent element (e.g., <body> or the header itself) when the navigation is open. You can then use standard CSS selectors to show/hide the appropriate buttons.
| /* apply theme classes to html as selected */ | ||
|
|
||
| if(navigator.install) { | ||
| pwas_link.hidden = false |
There was a problem hiding this comment.
This line will cause a ReferenceError if an element with the ID pwas_link does not exist in the DOM when this script runs. To prevent this, you should check if the element exists before attempting to access its properties.
if(typeof pwas_link !== 'undefined') {
pwas_link.hidden = false
}| <ul id="categories_list"> | ||
| { categories.map((category) => ( | ||
| <li> | ||
| <a href={ "/" + category.slug + "/1" }><img src={ "/icons/" + category.slug + ".png" } alt="" width="32" height="32">{ category.name }</a> |
There was a problem hiding this comment.
The src attribute for the <img> tag is constructed dynamically (e.g., /icons/books.png), but the corresponding icon files do not appear to be included in this pull request. This will result in broken images on the categories page. Please ensure all required image assets are added to the public/icons/ directory.
| background: buttonface; | ||
| } | ||
| body > header label, body > header button { | ||
| padding-left: var(--spacing-8);; |
There was a problem hiding this comment.
There is a redundant semicolon ;; at the end of this line. While most browsers will ignore it, it's best to remove it to maintain clean code.
padding-left: var(--spacing-8);| onclick={`document.getElementById('${uniqueId}').value = ''; document.getElementById('${uniqueId}').focus();`} | ||
| role="button" | ||
| tabindex="-1" | ||
| aria-label="Clear search" | ||
| onkeydown={`if(event.key === 'Enter' || event.key === ' '){event.preventDefault(); document.getElementById('${uniqueId}').value = ''; document.getElementById('${uniqueId}').focus();}`} | ||
| > |
There was a problem hiding this comment.
Using inline onclick and onkeydown attributes makes the code harder to read, maintain, and debug. It's better to separate concerns by moving this logic into a <script> tag. This also improves security by avoiding inline script execution.
I suggest adding a script to this component to handle these events programmatically.
| ```css | ||
| --surface-light-primary-gradient-start: #7543e3 /* Violet 60 */ | ||
| --surface-light-primary-gradient-end: #582acb /* Violet 70 */ | ||
| --surface-light-background-start: #fafafa /* White to */ |
There was a problem hiding this comment.
The comment for --surface-light-background-start seems incomplete. It says /* White to */, which can be confusing. For clarity, I suggest completing it to reflect the gradient's purpose, for example: /* White to strong white */.
| - **Metropolis Font**: This font is loaded locally. If font files aren't available, update `/src/styles/fonts.css` with proper font file paths. | ||
| - **No Tailwind**: This Astro project does NOT use Tailwind CSS. All styling must use regular CSS with design tokens. | ||
| - **Semantic First**: Always prefer semantic tokens over primitive tokens for consistency across light/dark modes. | ||
| - **Typography is Explicit**: Unlike Tailwind, typography styles are never inherited. Always set font-family, font-weight, font-size, and line-height explicitly. |
There was a problem hiding this comment.
The statement 'Unlike Tailwind, typography styles are never inherited' is technically incorrect for CSS, as properties like font-family, font-weight, etc., are inherited by default. This could cause confusion. A more accurate phrasing would be to recommend explicit declaration on components to avoid relying on inheritance.
I suggest rephrasing to something like: 'Unlike Tailwind, typography styles should not be inherited implicitly. Always set font-family, font-weight, font-size, and line-height explicitly on components to ensure consistency.'
| | `--heading-sm-size/line` | 32px / 36px | Sub-headings | | ||
| | `--heading-xs-size/line` | 24px / 28px | Card headings | | ||
| | `--heading-xxs-size/line` | 20px / 24px | Small headings | | ||
| | `--heading-xxxs-size/line` | 16px / 20px | Tiny headings (H2) | |
There was a problem hiding this comment.
The usage description for --heading-xxxs-size/line is 'Tiny headings (H2)'. Using H2 for a 'tiny' heading is unconventional and could be misleading for developers. Typically, H2 represents a major section heading. If this is a specific project convention, it might be worth clarifying. Otherwise, consider using a more appropriate heading level like H6 to better reflect its visual hierarchy.
sonofluiz
changed the title
2 participants
No description provided.