Feature/sc 109316/fe pro dashboard accessibility fixes #899
Conversation
|
This pull request has been linked to Shortcut Story #109316: [FE] Pro Dashboard Accessibility fixes. |
src/navigation/Nav.jsx
Outdated
| <img className="proIcon" alt={connections.label} src={CONNECTIONS_ICON} /> | ||
| <img | ||
| className="proIcon" | ||
| alt={connections && connections.label} |
There was a problem hiding this comment.
Is it possible that connections can be null or undefined? If so let's have a fallback string value here
There was a problem hiding this comment.
yep it's possible cause on pro-web connections not implemented and I just try to fix issue for update a lib version, it's not a part of my work
There was a problem hiding this comment.
I think it should be removed when connections will be implemented on pro-web, but idk who deals with this
There was a problem hiding this comment.
I see, it will fix the type error, but still will leave alt attribute blank. Why not to use ternary here? connections ? connections.label : 'pro-icon (or some other fallback)
src/forms/Button.jsx
Outdated
| ref={forwardedRef} | ||
| {...other} | ||
| > | ||
| {icon ? ( |
There was a problem hiding this comment.
It's really hard to read multiple ternary operators in jsx and I surprised that it is allowed in eslint rules. Can we move it to a separate render function?
There was a problem hiding this comment.
it's just a copypast from swarm repo
There was a problem hiding this comment.
we have the same ternary above ) from 2019
There was a problem hiding this comment.
I think it's a good chance to improve the code ;)
meetup-oleksandr-dudiuk
deleted the
feature/sc-109316/fe-pro-dashboard-accessibility-fixes
branch
Related issues
Fixes PIVOTAL-xxx
Description
Screenshots (if applicable)