-
Notifications
You must be signed in to change notification settings - Fork 0
feat(store): add images to store and cart items with persistence #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@Conductor-Codes review |
| <button | ||
| onClick={() => removeFromCart(idx)} | ||
| style={{marginLeft: 8}}> | ||
| Remove | ||
| </button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <button | |
| onClick={() => removeFromCart(idx)} | |
| style={{marginLeft: 8}}> | |
| Remove | |
| </button> | |
| <button | |
| onClick={() => removeFromCart(idx)} | |
| style={{marginLeft: 8}} | |
| aria-label={`Remove ${item.name} from cart`}> | |
| Remove | |
| </button> |
The "Remove" button lacks accessible context for screen reader users. Without additional context, screen reader users won't know which item the button will remove, making the interface confusing and potentially causing unintentional actions.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <button onClick={() => addToCart(product)} style={{marginTop: 8}}> | ||
| Add to Cart | ||
| </button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <button onClick={() => addToCart(product)} style={{marginTop: 8}}> | |
| Add to Cart | |
| </button> | |
| <button | |
| onClick={() => addToCart(product)} | |
| style={{marginTop: 8}} | |
| aria-label={`Add ${product.name} to Cart`}> | |
| Add to Cart | |
| </button> |
The "Add to Cart" button lacks an accessible name that includes the product being added. Screen reader users won't know which product they're adding to the cart, as the button text is the same for all products.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <Link href="/Store" style={{float: 'right', marginBottom: 16}}> | ||
| Back to Store | ||
| </Link> | ||
| {cart.length === 0 ? ( | ||
| <p>Your cart is empty.</p> | ||
| ) : ( | ||
| <ul style={{listStyle: 'none', padding: 0}}> | ||
| {cart.map((item, idx) => ( | ||
| <li | ||
| key={idx} | ||
| style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Link href="/Store" style={{float: 'right', marginBottom: 16}}> | |
| Back to Store | |
| </Link> | |
| {cart.length === 0 ? ( | |
| <p>Your cart is empty.</p> | |
| ) : ( | |
| <ul style={{listStyle: 'none', padding: 0}}> | |
| {cart.map((item, idx) => ( | |
| <li | |
| key={idx} | |
| style={{ | |
| <style jsx global>{` | |
| a:focus, button:focus { | |
| outline: 2px solid #007bff; | |
| outline-offset: 2px; | |
| } | |
| `}</style> | |
| <Link href="/Store" style={{float: 'right', marginBottom: 16}}> | |
| Back to Store | |
| </Link> | |
| {cart.length === 0 ? ( | |
| <p>Your cart is empty.</p> | |
| ) : ( |
All interactive elements (buttons and links) lack visible focus indicators. Without proper focus styles, keyboard users won't be able to see which element is currently focused when navigating, making the interface inaccessible for keyboard-only users.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | ||
| Go to Cart ({cart.length}) | ||
| </Link> | ||
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | ||
| {products.map((product) => ( | ||
| <div | ||
| key={product.id} | ||
| style={{ | ||
| border: '1px solid #eee', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| <Link href="/Cart" className="focusable-element" style={{float: 'right', marginBottom: 16}}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| borderRadius: 8, | |
| padding: 16, | |
| width: 220, | |
| }}> |
All interactive elements (buttons and links) lack visible focus indicators. This makes it impossible for keyboard users to see which element is currently focused, creating a significant accessibility barrier for people who cannot use a mouse.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | ||
| Go to Cart ({cart.length}) | ||
| </Link> | ||
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | ||
| {products.map((product) => ( | ||
| <div | ||
| key={product.id} | ||
| style={{ | ||
| border: '1px solid #eee', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| <Link href="/Cart" className="focusable-element" style={{float: 'right', marginBottom: 16}}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| borderRadius: 8, | |
| padding: 16, | |
| width: 220, | |
| }}> |
All interactive elements (buttons and links) lack visible focus indicators. This makes it impossible for keyboard users to see which element is currently focused, creating a significant accessibility barrier for people who cannot use a mouse.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | ||
| Go to Cart ({cart.length}) | ||
| </Link> | ||
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | ||
| {products.map((product) => ( | ||
| <div | ||
| key={product.id} | ||
| style={{ | ||
| border: '1px solid #eee', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| <Link href="/Cart" style={{float: 'right', marginBottom: 16}} className="no-bg-scrollbar" onFocus={(e) => e.target.style.outline = '2px solid #4299e1'} onBlur={(e) => e.target.style.outline = 'none'}> | |
| Go to Cart ({cart.length}) | |
| </Link> | |
| <div style={{display: 'flex', gap: 32, flexWrap: 'wrap'}}> | |
| {products.map((product) => ( | |
| <div | |
| key={product.id} | |
| style={{ | |
| border: '1px solid #eee', | |
| borderRadius: 8, | |
| padding: 16, | |
| width: 220, | |
| }}> |
All interactive elements (buttons and links) lack visible focus indicators. This makes it impossible for keyboard users to see which element is currently focused, creating a significant accessibility barrier for people who cannot use a mouse.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <img | ||
| src={product.image} | ||
| alt={product.name} | ||
| style={{ | ||
| width: '100%', | ||
| height: 140, | ||
| objectFit: 'cover', | ||
| borderRadius: 4, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <img | |
| src={product.image} | |
| alt={product.name} | |
| style={{ | |
| width: '100%', | |
| height: 140, | |
| objectFit: 'cover', | |
| borderRadius: 4, | |
| }} | |
| /> | |
| <img | |
| src={product.image} | |
| alt={product.name} | |
| width="220" | |
| height="140" | |
| style={{ | |
| width: '100%', | |
| height: 140, | |
| objectFit: 'cover', | |
| borderRadius: 4, | |
| }} | |
| /> |
Images without explicit width and height HTML attributes can cause layout shifts as the browser doesn't know the image dimensions until they load. This negatively impacts Cumulative Layout Shift (CLS), an important Core Web Vitals metric.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
| <img | ||
| src={item.image} | ||
| alt={item.name} | ||
| style={{ | ||
| width: 48, | ||
| height: 48, | ||
| objectFit: 'cover', | ||
| borderRadius: 4, | ||
| marginRight: 16, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <img | |
| src={item.image} | |
| alt={item.name} | |
| style={{ | |
| width: 48, | |
| height: 48, | |
| objectFit: 'cover', | |
| borderRadius: 4, | |
| marginRight: 16, | |
| }} | |
| /> | |
| <img | |
| src={item.image} | |
| alt={item.name} | |
| width={48} | |
| height={48} | |
| style={{ | |
| width: 48, | |
| height: 48, | |
| objectFit: 'cover', | |
| borderRadius: 4, | |
| marginRight: 16, | |
| }} | |
| /> |
Images without explicit width and height HTML attributes can cause layout shifts as the browser doesn't know the image dimensions until they load. This negatively impacts Cumulative Layout Shift (CLS), an important Core Web Vitals metric.
Review by Conductor
Is this review helpful? React 👍 or 👎 to let us know!
@Conductor-Codes review