10 year anniversary#15499
Conversation
corwintines
requested review from
minimalsm,
pettinarip and
wackerow
as code owners
❌ Deploy Preview for ethereumorg failed.
|
github-actions
Bot
added
content 🖋️
|
|
||
| const Page = async ({ params }: { params: Promise<{ locale: Lang }> }) => { | ||
| const { locale } = await params | ||
| const { adoptionCards, adoptionStyles } = use10YearAnniversary() |
There was a problem hiding this comment.
There is no state-management in here (use10YearAnniversary), so I wouldn't make this a hook (Next.js will yell are you for this in an async server-side function)... consider exporting from a data.tsx file in this folder instead
copy updates
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
well done @corwintines!
There are a few visual issues with the globe (that looks 🔥 btw) that I commented.
Other than that, I left a few observations as potential refactors and nice to have.
There was a problem hiding this comment.
The "10 years" title is not correctly centered
There was a problem hiding this comment.
Would be nice to have a more abstract CoundDown component placed in the src/components folder or just the functionality as a hook (useCountdown). I see a potential reusability for this functionality in the future.
There was a problem hiding this comment.
we need to disable the zoom (using scroll in this case)
There was a problem hiding this comment.
The other issue, that we already talked about but just as a track record, is related to the tooltips not closing when you scroll the page
| eventCategory: "10-year-anniversary", | ||
| }} | ||
| > | ||
| Read more |
There was a problem hiding this comment.
nice to have: we are displaying this button always but in most cases the whole text is already displayed, making this button useless. Could we display it only when necessary?
| }) | ||
|
|
||
| // loops over chars to morph a text to another | ||
| const morpher = (start: string, end: string): void => { |
There was a problem hiding this comment.
DRY 😢
nice to have: refactor Morpher to accept words, and chars (as optional) as props and reuse logic here.
There was a problem hiding this comment.
All good with this file living in this place as far as this page is temporary. If we are planning on reusing it, would be nice to move it to the data folder, as it is also not a component.
| rgba(231, 202, 200, 0.1) 82.88% | ||
| ); | ||
|
|
||
| --gradient-step-1: rgba(127, 127, 213, 0.20); |
There was a problem hiding this comment.
why is it call step-1? looking into the code, this looks like a new card or banner gradient, I'd align its name with the other similar gradients, --banner-secondary-gradient or --card-secondary-gradient.
Also, I'd encourage to reuse some of the already existing gradients cc @nloureiro @konopkja
| --card-gradient: linear-gradient(123deg, rgba(255, 255, 255, 0.20) 58.99%, rgba(174, 110, 203, 0.13) 104.04%); | ||
| --card-gradient-secondary: linear-gradient(95deg, rgba(211, 145, 242, 0.12) 0%, rgba(159, 43, 212, 0.12) 102.78%); | ||
| --card-gradient-secondary-hover: linear-gradient(95deg, rgba(211, 145, 242, 0.2) 0%, rgba(159, 43, 212, 0.2) 102.78%); | ||
| --ten-year-gradient: linear-gradient(100deg, #F6C9EA 55.38%, #C7A9F1 110.54%); |
There was a problem hiding this comment.
Similar point with this gradient and variable name. Im fine adding it as long as this is a temporary page. Otherwise, would encourage you to create a better name to be reused later in other components.
cc @nloureiro @konopkja would be nice to define all the gradients we have in the DS to have consistency in future interations. current gradients
| setPosition({ x, y }) | ||
| }) | ||
|
|
||
| const [prefersReducedMotion] = useMediaQuery([ |
There was a problem hiding this comment.
|
Estoy bien |
|
|
6 participants
Description
Related Issue