Conversation
The collapsible and dismissable controllers persisted their state in `callout_block_*` cookies, so every page load shipped it back to PyPI in the request headers for state only the browser ever reads. Move both to localStorage, following the notification controller. This removes the `setting="global"` attribute along with the cookies. It existed to choose between a `path=/` cookie and a path-scoped one, and localStorage is origin-wide, so it no longer means anything. Every collapsible already set it, so those are unaffected. The dismissable callouts did not, and their dismissal was therefore scoped to the URL directory - dismissing the same explanatory callout once per project you own. It now sticks the first time. Existing cookies are not migrated: the state is "has read this help text", so the cost of a callout reappearing once is a single click, which is cheaper than a migration shim someone has to remember to delete. Refs pypi#14950
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14950
Moves the collapsible and dismissable controllers from
callout_block_*cookiesto localStorage, following
notification_controller.js. This state is only everread by the browser, so sending it up in the headers on every request bought
nothing.
One behaviour change, and it is the interesting part
data-*-setting="global"existed to choose between apath=/cookie and apath-scoped one. localStorage is origin-wide, so it no longer means anything and
is removed along with the two template attributes.
What it was doing turns out to matter, though. Every collapsible already set it,
so those are unaffected. No dismissable did — so a dismissed callout was scoped
to the URL directory, which means the same explanatory callout had to be
dismissed again on every project you own. That reads like an accident rather
than a decision, and it now sticks the first time. Happy to preserve the old
scoping if it was deliberate.
Existing cookies are not migrated. The state is "has read this help text", so a
callout reappearing once costs a click, which seemed cheaper than a migration
shim someone has to remember to delete.
Not done
The issue floats converting all three controllers into one. That touches
notification_controllerand its key naming too, so I left it — happy to followup if you want it.
Testing
npm test: 111 pass underTZ=UTC, both controllers at 100%.npm run lintand djlint clean.
Both test files are rewritten. The old collapsible test seeded a
callout_block_settings_collapsedcookie against a fixture whose identifier wasproject_roles, so the controller never saw it and the assertion passed for thewrong reason. The new tests fail against the cookie implementation (5 failures)
and pass against this one.