Closed interim17 closed 5 months ago
St.:grey_question: |
Category | Percentage | Covered / Total |
---|---|---|---|
π‘ | Statements | 73.27% | 603/823 |
π‘ | Branches | 68.38% | 80/117 |
π΄ | Functions | 40.31% | 79/196 |
π‘ | Lines | 71.73% | 538/750 |
99 tests passing in 7 suites.
Report generated by π§ͺjest coverage report action from c46d68690959ff1286e5ffda1abb9a2626f96b3d
Time estimate or Size
Review time: small/medium. Bigish diff, but lots of deleted code and 1:1 swaps of buttons for new custom components.
Reading
NavButton
andNavButtonWIthTooltip
and their stylesheet will tell you most of what you need to know.Problem
Buttons in the header are not styled correctly! They also aren't tab navigable, and could be more DRY.
Actions buttons need disabled state styling, primary and secondary need hover states, "Simularium Home" needs action button styling, and all buttons need consistent focus/hover behavior.
App header needed spacing tweaks on elements. The media queries are out of date with current design, responsive design update out of scope.
Figma SSOT
Solution
Solved: styling.
Improved but not solved: accessibility, DRY concerns.
NavButton
andNavButtonWithTooltip
All buttons in the header are now one of these.
NavButton
is primarily a theming wrapper, styles are now correct for different button types (primary, seconday, action), and for hover/focus states which makes tab navigation visually apparent. Next steps for that: make the dropdowns work; this refactor takes us a step in the right direction. In looking at accessible drop-downs I think implementing this component will be useful in the dropdown menu itemsNavButtonWIthTooltip
takes care of tooltip logic.I could make these all one component, but I think it's nice to have it separated so the props are clear, and there isn't conditional rendering of tooltip. It would be easy to consolidate if reviewers have feedback preferring consolidation.
Miscellany:
Tooltip offsets: using the disabled prop on the buttons added a span that made it necessary to fuss with the tooltip offsets. I think it's easier to just disable the buttons by disabling pointer events with CSS and not use the antd prop at all. Then we can use one offset constant all the time.
The "Simularium Home" link was a react router
Link
but UX wants it styled identical to an action button, and with the same tab navigation behavior. So I did a quick shift of theAppHeader
from a class based component to functional so that I could make use ofuseHistory
.AppHeader
had no lifecycle methods, the refactor is trivial.Share and Download components no longer need classNames or stylesheets.
In Download component I refactored a bit to remove arguments and what I thought were redundant checks that are handled by
isDisabled
constant.Steps to Verify:
Screenshots (optional):