Feat(banner) hide #7058

AugustinMauroy · 2024-09-24T10:09:35Z

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

I have read the Contributing Guidelines and made commit messages that follow the guideline.
I have run npm run format to ensure the code follows the style guide.
I have run npm run test to check if all tests are passing.
I have run npx turbo build to check if the website builds without errors.
I've covered new added functionality with unit tests if necessary.

vercel · 2024-09-24T10:09:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	Sep 26, 2024 7:29am

bmuenzenmeyer

functionally this works on the preview pretty simply
found one typo
have one small concern about relying on good JSON data in site.json

apps/site/components/Common/Banner/index.tsx

bmuenzenmeyer · 2024-09-26T03:46:00Z

apps/site/components/Common/Banner/index.tsx

 };

 const Banner: FC<PropsWithChildren<BannerProps>> = ({
  type = 'default',
  link,
+  onHiding,


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

packages/i18n/locales/en.json

bmuenzenmeyer · 2024-09-26T03:55:43Z

apps/site/components/withBanner.tsx


 const WithBanner: FC<{ section: string }> = ({ section }) => {
  const banner = siteConfig.websiteBanners[section];
+  const UUID = twoDateToUIID(banner.startDate, banner.endDate);


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.

We can call it generatekeyForBanner. But UIID mean Universally unique identifier.

bmuenzenmeyer · 2024-09-26T04:01:46Z

apps/site/util/stringUtils.ts

+  const date1String = date1.split('T')[0];
+  const date2String = date2.split('T')[0];


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

I'll have to reread the split docs because split automatically gives you a table that can be unique.

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]>

apps/site/hooks/react-client/useLocalStorage.ts

ovflowd · 2024-09-30T13:28:34Z

apps/site/components/withBanner.tsx


 const WithBanner: FC<{ section: string }> = ({ section }) => {
  const banner = siteConfig.websiteBanners[section];
+  const UUID = twoDateToUIID(banner.startDate, banner.endDate);


This feels so weird.

ovflowd

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.

AugustinMauroy · 2024-10-01T09:32:29Z

also not OK, not to mention the whole hacky and weird UUID logic.

We can base the logic on dates and not a UIID.

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.

I understand your point of view. I sent a message on slack to get a sample of opinions. And based on that:

Either we abandon the changes.
Or we find a clean solution to implement it.

Note that Caner had suggested using cookies, which has two advantages:

the function for reading them is provided by next
there's no more layout shift
But we don't know what this means from a legal point of view
ref: Feat(banner) hide #7058 (comment)

ovflowd · 2024-10-01T09:45:01Z

also not OK, not to mention the whole hacky and weird UUID logic.

We can base the logic on dates and not a UIID.

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.

I understand your point of view. I sent a message on slack to get a sample of opinions. And based on that:

Either we abandon the changes.

Or we find a clean solution to implement it.

Note that Caner had suggested using cookies, which has two advantages:

the function for reading them is provided by next

there's no more layout shift

But we don't know what this means from a legal point of view

ref: #7058 (comment)

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 😅

ovflowd · 2024-10-01T20:20:39Z

I forgot to comment regarding:

But we don't know what this means from a legal point of view

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.

bmuenzenmeyer · 2024-10-25T03:22:42Z

are we abandoning this for now then?

AugustinMauroy · 2024-10-26T09:41:33Z

are we abandoning this for now then?

I wanted to see how nextra and docausorus work. But I haven't taken the time for that yet.

ovflowd · 2024-11-20T19:49:11Z

I'm closing this for now, we can always circle back.

AugustinMauroy and others added 4 commits February 17, 2024 16:19

feat(banner): add x to hide (design)

b3b0bca

feat(banner): add logic to hide

d220af9

Merge branch 'main' into feat(banner)-hide

2018e3d

feat(banner): clean + fix

8457dc5

AugustinMauroy marked this pull request as ready for review September 24, 2024 10:09

AugustinMauroy requested a review from a team as a code owner September 24, 2024 10:09

aymen94 approved these changes Sep 24, 2024

View reviewed changes

bmuenzenmeyer requested changes Sep 26, 2024

View reviewed changes

AugustinMauroy and others added 2 commits September 26, 2024 09:25

fix: typo

1a4b548

Co-authored-by: Brian Muenzenmeyer <[email protected]> Signed-off-by: Augustin Mauroy <[email protected]>

change name

1b26606

Co-authored-by: Brian Muenzenmeyer <[email protected]> Signed-off-by: Augustin Mauroy <[email protected]>

vercel bot deployed to Preview September 26, 2024 07:29 View deployment

canerakdas reviewed Sep 26, 2024

View reviewed changes

apps/site/hooks/react-client/useLocalStorage.ts Show resolved Hide resolved

ovflowd reviewed Sep 30, 2024

View reviewed changes

ovflowd requested changes Sep 30, 2024

View reviewed changes

ovflowd closed this Nov 20, 2024

ovflowd deleted the feat(banner)-hide branch November 20, 2024 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(banner) hide #7058

Feat(banner) hide #7058

AugustinMauroy commented Sep 24, 2024

vercel bot commented Sep 24, 2024 •

edited

Loading

bmuenzenmeyer left a comment

bmuenzenmeyer Sep 26, 2024

bmuenzenmeyer Sep 26, 2024

AugustinMauroy Sep 26, 2024

bmuenzenmeyer Sep 26, 2024

AugustinMauroy Sep 26, 2024

AugustinMauroy Sep 26, 2024

ovflowd Sep 30, 2024

ovflowd left a comment

AugustinMauroy commented Oct 1, 2024

ovflowd commented Oct 1, 2024 •

edited

Loading

ovflowd commented Oct 1, 2024

bmuenzenmeyer commented Oct 25, 2024

AugustinMauroy commented Oct 26, 2024 •

edited

Loading

ovflowd commented Nov 20, 2024

		const date1String = date1.split('T')[0];
		const date2String = date2.split('T')[0];

Feat(banner) hide #7058

Feat(banner) hide #7058

Conversation

AugustinMauroy commented Sep 24, 2024

Description

Related Issues

Check List

vercel bot commented Sep 24, 2024 • edited Loading

bmuenzenmeyer left a comment

Choose a reason for hiding this comment

bmuenzenmeyer Sep 26, 2024

Choose a reason for hiding this comment

bmuenzenmeyer Sep 26, 2024

Choose a reason for hiding this comment

AugustinMauroy Sep 26, 2024

Choose a reason for hiding this comment

bmuenzenmeyer Sep 26, 2024

Choose a reason for hiding this comment

AugustinMauroy Sep 26, 2024

Choose a reason for hiding this comment

AugustinMauroy Sep 26, 2024

Choose a reason for hiding this comment

ovflowd Sep 30, 2024

Choose a reason for hiding this comment

ovflowd left a comment

Choose a reason for hiding this comment

AugustinMauroy commented Oct 1, 2024

ovflowd commented Oct 1, 2024 • edited Loading

ovflowd commented Oct 1, 2024

bmuenzenmeyer commented Oct 25, 2024

AugustinMauroy commented Oct 26, 2024 • edited Loading

ovflowd commented Nov 20, 2024

vercel bot commented Sep 24, 2024 •

edited

Loading

ovflowd commented Oct 1, 2024 •

edited

Loading

AugustinMauroy commented Oct 26, 2024 •

edited

Loading