-
-
Notifications
You must be signed in to change notification settings - Fork 150
Created New Feature Banner and Added to Home Page #2037
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Mephistic
left a comment
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.
We generally don't review Draft PRs (so apologies in advance if any of these comments are already on your radar), but I was going over other PRs anyway and thought a few notes might be helpful.
| endDate: Date | ||
| children: React.ReactElement | ||
| }) { | ||
| const isOpen = localStorage.getItem("isBannerClosed") |
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.
I believe there's a useLocalStorage hook provided by use-hooks that could help simplify this: https://usehooks-ts.com/react-hook/use-local-storage
| @@ -0,0 +1,48 @@ | |||
| import { Row } from "react-bootstrap" | |||
| import styles from "./AnouncementBanner.module.css" | |||
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.
I don't see this module.css file in the PR - does it exist? If not, we should remove this import.
If so, you've hit an embarassing point for us - we made a point to move MAPLE away from CSS modules to styled-components a while ago, but missed the home page in the transition 🤦 (I think someone picked up the ticket before leaving the project). If this needs more complicated styles, we should prefer styled-components.
That said, I think this already looks great on dev with just the utility classes.
| } | ||
|
|
||
| const now: Date = new Date() | ||
| console.log(open, isOpen) |
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.
nit: Please remember to remove the log statement before we merge
| <p className="mb-0"> | ||
| <span className="fw-bold">New on MAPLE:</span>{" "} | ||
| <span> | ||
| Looking to learn about the latest action in a committee? |
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.
nit: For raw copy text like this, we should extract strings for I18n:
- In the component itself, we should either use a
useTranslationhook (e.g.const { t } = useTranslation("common")) or a<Trans>component - Where we would have text previously, we should provide the hook a translation key (e.g. instead of "New on MAPLE:",
t("announcement.heading")) - In the
public/locales/enfolder, we should add a mapping of translation keys to raw copy text in a relevant JSON file (e.g.announcement: { heading: "New on MAPLE:"})- By default, the homepage just uses
common.json. IMO that's fine for this - we can make something more announcement specific later when we want to add a second announcement.
- By default, the homepage just uses
There's examples of how we handle this in MAPLE throughout the codebase - useTranslation is easily searchable, and the <Trans> component is mainly used for text that contains hyperlinks (like
maple/components/hearing/HearingDetails.tsx
Line 137 in f1a07df
| <Trans |
(Translations aren't fully wired up yet, but we want the text to be ready to go when we're ready to move on that project)
Mephistic
left a comment
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.
LGTM - thanks for the quick turn-around on this!
Summary
This changes adds a notification banner to the home page announcing the new transcription banner. The banner can be closed using the x button and will remain closed for future visits after it has been closed once. The banner will stop appearing automatically after March 1, 2026.
closes #2030
Screenshots
Steps to test/reproduce
To restore the banner after closing, clear the local storage for the page.