Feat(banner) hide #7058
Feat(banner) hide #7058
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| }; | ||
|
|
||
| const Banner: FC<PropsWithChildren<BannerProps>> = ({ | ||
| type = 'default', | ||
| link, | ||
| onHiding, |
There was a problem hiding this comment.
I suggest a different name here. onHiding to me implies it's a handler, when it's really a conditional state or mode that the banner can opt-into.
isHideable would not the boolean nature, and nod to the story name Hideable
|
|
||
| const WithBanner: FC<{ section: string }> = ({ section }) => { | ||
| const banner = siteConfig.websiteBanners[section]; | ||
| const UUID = twoDateToUIID(banner.startDate, banner.endDate); |
There was a problem hiding this comment.
nit: at first glance, I thought we were using the uuid package here due to the name. Not sure if it needs to change, but calling attention to the momentary confusion. I think the way you did this was clever overall.
There was a problem hiding this comment.
We can call it generatekeyForBanner. But UIID mean Universally unique identifier.
| const date1String = date1.split('T')[0]; | ||
| const date2String = date2.split('T')[0]; |
There was a problem hiding this comment.
nothing seems to enforce this as a a proper date string over within site.json, which could lead to bugs or errors here splitting on T
There was a problem hiding this comment.
I'll have to reread the split docs because split automatically gives you a table that can be unique.
There was a problem hiding this comment.
But we can also use new Date and throw error if not right date
Co-authored-by: Brian Muenzenmeyer <[email protected]> Signed-off-by: Augustin Mauroy <[email protected]>
Co-authored-by: Brian Muenzenmeyer <[email protected]> Signed-off-by: Augustin Mauroy <[email protected]>
|
|
||
| const WithBanner: FC<{ section: string }> = ({ section }) => { | ||
| const banner = siteConfig.websiteBanners[section]; | ||
| const UUID = twoDateToUIID(banner.startDate, banner.endDate); |
There was a problem hiding this comment.
I'm not sure about this PR. Sure, maybe being able to close the banner is nice... But how many people actually want this / or are frustrated with the banner?
Becazse as a technical change, I'm not a fan of using local storage just for this, and there are many other little technical details that are ehh, also not OK, not to mention the whole hacky and weird UUID logic.
I just overall don't feel confident with these changes and how much garbage we are adding just to have a single X button. The benefits are just not there.
We can base the logic on dates and not a UIID.
I understand your point of view. I sent a message on slack to get a sample of opinions. And based on that:
Note that Caner had suggested using cookies, which has two advantages:
|
Im fine with the feature -- Im just pretty much saying your code is not good, and I won't allow it to get merged on the state it is, sorry. I might have more bandwidth in the future; But still I'm not sure about how much value we get from hiding banners. And this is such a small change that genuinely speaking bothering core collaborators to interfere here is overkill and unnecessary. Some banners should just stay there, and I don't think you should make an UUID based on the dates. You could simply create a hash based on the banner entry itself (the json blob) I also don't have the time at the moment to propose code changes or to coach you here. For me it is more valuable for Caner to take over your PR if we decide to move forward with this. Which I doubt we will. Sorry, Augustin 😅 |
|
I forgot to comment regarding:
Just a FYI although it is a cookie, we could consider these to be functional 1st party cookies as they store user preferences. Nothing is really done with them, I doubt on a GDPR standpoint anything needs to be done. Just so you know, the "legal concerns" you have are not limited to cookies, they also apply to localStorage. |
Description
A little while ago we discussed hiding the banner, and since then there have been no major objections, so I've opened this pr to implement it.
Related Issues
close #6292
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.