Added notice #119
Added notice #119
Conversation
dbayly-freshworks
commented
Jan 4, 2022
•
dbayly-freshworks
commented
There was a problem hiding this comment.
We should be able to match designs a bit closer with some changed margins, border radius and icon size. I think it could be worthwhile to abstract this into an alert component (with a single variant for now)
Placing the alert in the same div as the checkbox should help with spacing and will prevent the extra divider from showing
Co-authored-by: arranfw <71518072+arranfw@users.noreply.github.com>
apps/web/src/components/Advisory.tsx
Outdated
| import { faExclamationCircle } from '@fortawesome/free-solid-svg-icons'; | ||
| import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; | ||
|
|
||
| export const Advisory: React.FC = ({ children }) => { |
There was a problem hiding this comment.
Can we call this Alert to match the BC gov term? https://developer.gov.bc.ca/Design-System/Alert-Banners
There was a problem hiding this comment.
Sure, but I feel weird about it for a couple reasons.
- Alert is already an html tag name, and does something very different
- The BC gov alert component has a lot more features/functions that what this component does. It feels like either a mismatched component or a fair bit of scope creep.
There was a problem hiding this comment.
It's just different variants and accessibility features isn't it? I think we should implement as much as is reasonable, accessibility stuff is pretty important but we don't need the other variants for now (warning/error/etc..)
…shifted the Alert up one div
| @@ -22,6 +23,7 @@ import { | |||
| deploymentDurationOptions, | |||
| placementOptions as allPlacementOptions, | |||
| } from '../validation/preferences'; | |||
| import { Alert } from '../../Alert'; | |||
There was a problem hiding this comment.
Last small comment, could you add alert to components/index.ts and add this import to the other component imports on L1?
