Track if users has dismissed a version warning banner for a given version.

[Carreau]

See #1384

If a user dismiss the banner it should apply to all pages.
One remaining questions is :

• does it apply to all versions ?

This implement a dismiss button and store in local storage which version and when the user has dismissed it.

This is enough informations to refine the logic later with what/when we want to show.

This also has a rough implementation of not showing banner for current dismissed version for the next 14 days following any banner dismissal.

[drammock]

thanks for tackling this! My feelings:

1. close button should not be the letter "x", should be 🗙 (U+1f5d9, "cancellation X") or https://fontawesome.com/icons/xmark?f=classic&s=solid. Not sure about horizontal alignment; I was envisioning it right-aligned (near the viewport edge) but no strong feeling there
2. I'm not sure how to choose the duration of the dismissal. 1 month feels too long to me, and really anything other than "the duration of this browsing session" feels arbitrary. So I lean toward not doing any calendar-time-based memory.
3. Generally I think it's a good idea to "dismiss for all versions". E.g. for version warning banners, if someone's on an old version and then goes to a different old version we can probably assume they did it on purpose / are specifically seeking info about older APIs, so we probably don't need to warn them again. For general announcement banners, I think it's even clearer that it's OK to dismiss once for all versions.

[Carreau]

1. close button should not be the letter "x", should be 🗙 (U+1f5d9, "cancellation X") or https://fontawesome.com/icons/xmark?f=classic&s=solid. Not sure about horizontal alignment; I was envisioning it right-aligned (near the viewport edge) but no strong feeling there

Same feeling here, just did not had time/skills to do that.

2. I'm not sure how to choose the duration of the dismissal. 1 month feels too long to me, and really anything other than "the duration of this browsing session" feels arbitrary. So I lean toward not doing any calendar-time-based memory.

This is why I store the date of dismissal instead of storing (date+1month), so that we (as developers) can refine that later. The date could be treated as a boolean, or we can include a footer to reset or change the duration per user. I'm not attached to a particular logic. I just think it's good to give us the option of ignoring a previous dismissal.

3. Generally I think it's a good idea to "dismiss for all versions". E.g. for version warning banners, if someone's on an old version and then goes to a different old version we can probably assume they did it on purpose / are specifically seeking info about older APIs, so we probably don't need to warn them again. For general announcement banners, I think it's even clearer that it's OK to dismiss once for all versions.

I agree, I think there is basically 3 groups of versions:

• devs/unreleased
• stable(s?) – this one should have no waring anyway
• older

The main question is do we want to separate devs from older or treat both same.

[Carreau]

1. close button should not be the letter "x", should be 🗙

does not seem to show properly on my machine, so I went for the fa-icon.

I'm not sure how to make the close button go on the right edge of the page without sending the test of the banner on the left or adding a bunch of intermediate divs (would that be ok ? )

The way the annoucement banner are done is completely different, and is in the Jinja templates.

Should the two kind of banner creating logic be unified ?

[drammock]

The main question is do we want to separate devs from older or treat both same.

I think ideally they would be treated differently (?) but I'm content with either way if one of them is much harder to do.

I'm not sure how to make the close button go on the right edge

putting classes ms-auto me-auto on the bd-header-announcement__content div (plus maybe class me-3 or so on the close button) sort of works, but it aligns the X to the container (which is not the full viewport width on large screens). But maybe that's good enough for now?

The way the annoucement banner are done is completely different, and is in the Jinja templates.
Should the two kind of banner creating logic be unified ?

Ideally yes. But that could be another PR.

[Carreau]

Oh, yes, the ms- and me- work great. I haven't added the me-3 to the button as the css inspector says it is ignore/does not apply.

I think the "wrong" position of the close button is because the banner use custom margin/padding. I think it probably could use the same css values/classes as the nav bar with .bd-page-width, but that will requires a bit of refactor and extra divs I belive.

Maybe let's leave the for refactor that consolidate the various banners code.

[drammock]

@Carreau I rebased to remove merge conflict, and pushed a few extra commits to tweak the JS (most notably I changed the timeout from 1 month to 14 days). Commit msgs are fairly descriptive of what else I did.

[github-actions]

Coverage report

This PR does not seem to contain any modification to coverable code.

[Carreau]

All seem good to me, I was just wondering why let instead of const, but it does not matter much.

[Carreau]

No, getDate() is wrong, it returns the day of the month, if you dismiss anytime past the second half of the month, it will never com back

[Carreau]

I fixed the .getDate() that was wrong by actually calculating the . I think 14 days is a bit short, but we can still change later.

The setMonth(getMonth() -1) (or setDay(getDay() -14)) works as it does the wrap around and decrement the year (resp Month). But computing the delta was not ok because it's bound in 1-12 (1-31).

[drammock]

The setMonth(getMonth() -1) (or setDay(getDay() -14)) works as it does the wrap around and decrement the year (resp Month). But computing the delta was not ok because it's bound in 1-12 (1-31).

Thanks for catching that, I didn't test carefully enough

[Carreau]

This picked up conflicts... :-(

[Carreau]

Rebased ( and squashed for simplicity) and a few extra changes in second commit, in particular bd-header-version-warning is now an ID as it is supposed to be only one.

[drammock]

just a few last tweaks; if you're happy I think it's time to merge

[Carreau]

Ok, let's try.