done! - #11
done!#11sabuza-bot wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the skincare product advisor UI to support selecting products, persisting selections/chat history in localStorage, and generating + continuing a routine conversation via a Cloudflare Worker endpoint.
Changes:
- Added selected-products state management (chips, clear-all, persistence) and routine generation / follow-up chat flow.
- Implemented chat history rendering with basic assistant message formatting (headings + lists) and storage restore on load.
- Refreshed page structure and styling (new palette, section labels, product cards with detail popovers, updated typography).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
style.css |
Large visual redesign (palette variables, product cards, selected chips, chat styling, responsive rules). |
script.js |
Adds selection + persistence, Worker-backed chat/routine generation, and UI rendering for products/selected/chat. |
index.html |
Adds section labels, selected-products controls (clear + generate), removes secrets.js, updates fonts. |
.gitignore |
Ignores js/secrets.js in addition to secrets.js. |
| selectedProductsList.innerHTML = selectedProducts | ||
| .map( | ||
| (product) => ` | ||
| <div class="selected-chip"> | ||
| <span>${product.name}</span> | ||
| <button | ||
| type="button" | ||
| class="selected-chip-remove" | ||
| data-product-id="${product.id}" | ||
| aria-label="Remove ${product.name}" | ||
| > | ||
| × | ||
| </button> | ||
| </div> | ||
| `, | ||
| ) | ||
| .join(""); |
There was a problem hiding this comment.
Product names are interpolated directly into innerHTML (including into the aria-label). If products.json ever contains characters like quotes or markup, this can break attributes or enable DOM injection. Prefer building these chips with createElement + textContent (and set aria-label via setAttribute), or at minimum HTML-escape product.name before inserting.
| selectedProductsList.innerHTML = selectedProducts | |
| .map( | |
| (product) => ` | |
| <div class="selected-chip"> | |
| <span>${product.name}</span> | |
| <button | |
| type="button" | |
| class="selected-chip-remove" | |
| data-product-id="${product.id}" | |
| aria-label="Remove ${product.name}" | |
| > | |
| × | |
| </button> | |
| </div> | |
| `, | |
| ) | |
| .join(""); | |
| // Clear the current list before rebuilding it safely with DOM methods. | |
| selectedProductsList.innerHTML = ""; | |
| selectedProducts.forEach((product) => { | |
| // Create the chip wrapper. | |
| const chip = document.createElement("div"); | |
| chip.className = "selected-chip"; | |
| // Add the product name as text, not HTML. | |
| const name = document.createElement("span"); | |
| name.textContent = product.name; | |
| // Create the remove button safely. | |
| const removeButton = document.createElement("button"); | |
| removeButton.type = "button"; | |
| removeButton.className = "selected-chip-remove"; | |
| removeButton.dataset.productId = product.id; | |
| removeButton.setAttribute("aria-label", `Remove ${product.name}`); | |
| removeButton.textContent = "×"; | |
| // Put the pieces together and add them to the page. | |
| chip.appendChild(name); | |
| chip.appendChild(removeButton); | |
| selectedProductsList.appendChild(chip); | |
| }); |
| aria-describedby="product-description-${product.id}" | ||
| aria-label="Select ${product.name}" | ||
| > | ||
| <img src="${product.image}" alt="${product.name}"> | ||
| <div class="product-info"> |
There was a problem hiding this comment.
Multiple product fields are interpolated directly into HTML/attributes (aria-label, src, alt) via innerHTML. To avoid attribute-breaking and DOM injection risks, build the card DOM with createElement/textContent (and set attributes explicitly), or escape product values before interpolation.
| role="button" | ||
| tabindex="0" | ||
| aria-pressed="${selectedProductIds.has(product.id)}" | ||
| aria-describedby="product-description-${product.id}" |
There was a problem hiding this comment.
aria-describedby points to the full description popover even when it’s visually collapsed (opacity/pointer-events). Because the description isn’t actually hidden from assistive tech, screen readers may read a long description every time the card receives focus. Consider removing aria-describedby from the card (or only setting it when expanded) and toggle hidden/aria-hidden on the popover based on aria-expanded.
| aria-describedby="product-description-${product.id}" |
| const descriptionToggleBtn = e.target.closest(".description-toggle"); | ||
|
|
||
| if (descriptionToggleBtn) { | ||
| const descriptionCard = descriptionToggleBtn.closest(".product-card"); | ||
| const isExpanded = descriptionCard.classList.toggle("description-expanded"); |
There was a problem hiding this comment.
The click handler only excludes clicks on the “View details” button. Clicking inside the expanded description popover (or anywhere inside .description-control) will still bubble and toggle product selection, which is surprising UX. Add a guard to ignore clicks originating from the description area (or stop propagation from the popover) so reading details doesn’t change selection state.
| if (numberedItemMatch || bulletItemMatch) { | ||
| if (!isListOpen) { | ||
| html += "<ul>"; | ||
| isListOpen = true; | ||
| } |
There was a problem hiding this comment.
Numbered list items are detected (numberedItemMatch) but rendered inside a <ul>, which removes the numbering and contradicts the “numbered steps” output format you prompt for. Use an <ol> for numbered items (and keep <ul> for bullet items), or preserve the original numbers when rendering.
No description provided.