Add global bar component from static #4538
Merged
Conversation
This file contains 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
andysellick
force-pushed
the
global-bar-add
branch
from
1a9bef4 to
b37b709
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
526b29c to
29f7075
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
29f7075 to
ab6b041
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
1b40b45 to
8b8b528
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
8b8b528 to
2ab6a55
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
2ab6a55 to
b6af945
Compare
|
Have approved the Percy build as it's helpful for me to see if any tests fail beyond that, and the Percy diff is a whole new page for this component and therefore isn't useful for this PR anyway. |
andysellick
force-pushed
the
global-bar-add
branch
from
9219434 to
a45f601
Compare
andysellick
force-pushed
the
global-bar-add
branch
from
a45f601 to
7b9ae5a
Compare
- also make the stylesheet compile (was unable to find mixins) - and move global bar helper code into cookie functions
- component is almost certainly not working correctly yet, but at least stop it from breaking all the other tests - adds a placeholder spec file for the component template, will be filled in later
- was part of the original component in static, but lack of it only caused an error once trying to render the component guide page - modified a bit from the original, had to require active_support, rewrote minitest to rspec, added timecop, set timezone in tests, configure as app helper
- originally copied across and modified to preserve original code, but we don't need this
- remove option to pass a link for the non-title text as not currently needed - rename options to just title and text, simpler - remove show banner option, use existence of title as basis for displaying banner - make banner appear in component guide - write some rspec tests
- renaming the component from to global banner - no changes to files (will obviously break everything) just renaming for now
- updates all the code that refers to `global-bar` or `globar_bar` or `globalBar` (etc.) to match the new name of the component - does not update the GA4 code in terms of what is passed to GA4, because that still expects to receive something called `global bar` and if we change that it's going to mess up all the analytics
- use correct BEM syntax - remove unused styles (checked and they don't seem to exist anywhere else in alphagov)
- click handling is handled by a dismiss link, which is no longer part of the banner, so this code isn't ever used - the tests were all passing because they're based on a copied snippet of markup that isn't up to date
- previously prevented the banner from appearing on specific URLs - this will be handled instead by the banners gem
- relates to the additional content, which isn't part of the banner anymore, and the dismiss link, which also doesn't exist
- not complete, some of the code happens in the wrong order and repeats itself so still requires refactoring, but tests should at least pass for now - also make banner version a thing that gets passed to the component and read from a data attribute by the JS, instead of being hard coded in the init file
- stop setting a visible class on the HTML element and move it to on the component itself
- thanks to @AshGDS
andysellick
force-pushed
the
global-bar-add
branch
from
1571cb2 to
c7c25f8
Compare
- expand test coverage - improve test names
andysellick
force-pushed
the
global-bar-add
branch
from
c7c25f8 to
21d779d
Compare
andysellick
changed the title
This was referenced Jan 28, 2025
Merged
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.
What
Copies the code for the global banner from
static, with modifications. See commit history for details, but modifications include:Note that this will be a breaking change as it changes the name of the component from
global-bartoglobal-banner. This has been changed everywhere in this repo including in the GA4 code, except for the bit where we pass data to GA4, where we keep it as global-bar because that's what GA4 expects to receive.Corresponding PR to remove this from static: alphagov/static#3561
Why
We want to use the global banner in the new banners gem and we're working towards retiring
static, so this is the logical place for the component to live.Visual Changes
Trello card: https://trello.com/c/WtPkMQFM/386-move-global-bar-component-from-static-into-publishing-components-gem