Conversation
minaelee
left a comment
minaelee
left a comment
There was a problem hiding this comment.
LGTM - though looks like linkcheck is failing for reasons unrelated to this PR. Thanks for making this update!
geoffreynyaga
force-pushed
the
update-cookie-banner
branch
from
5bc0f14 to
ce8a9fb
Compare
|
|
||
| # On pull requests, HEAD^1 will always be the merge base, so consider that diff for formatting. | ||
| git diff -U0 --no-color HEAD^1 \ | ||
| | grep -vE '^\+\+\+ b/tests/unit/test_data/formatters/' \ |
There was a problem hiding this comment.
I remember removing this line in favor of a dedicated .clang-format file in the tests/unit/test_data_formatters folder. Is this a rebase mistake?
There was a problem hiding this comment.
@xmkg I believe this might be a rebase mistake. If you check my last commit, I was trying to fix the CI errors that arose after the rebase, I would recommend you push a commit to fix the clang-format issues.
There was a problem hiding this comment.
I have fixed this
| # On pull requests, HEAD^1 will always be the merge base, so consider that diff for formatting. | ||
| git diff -U0 --no-color HEAD^1 \ | ||
| | grep -vE '^\+\+\+ b/tests/unit/test_data/formatters/' \ | ||
| | grep -vE '^\+\+\+ b/docs/\.sphinx/' \ |
There was a problem hiding this comment.
Instead of this, you can add a .clang-format file into the docs/.sphinx path with the following content:
DisableFormat: true
There was a problem hiding this comment.
I have made the changes
| - name: Install clang-format | ||
| shell: bash | ||
| run: sudo apt-get install --no-install-recommends --yes clang-format-$SNAP_CLANG_VERSION | ||
| run: sudo apt-get install --no-install-recommends --yes clang-format-$SNAP_CLANG_VERSION clang-tools-$SNAP_CLANG_VERSION |
There was a problem hiding this comment.
Is there a reason to install the clang-tools on top of clang-format?
There was a problem hiding this comment.
@xmkg
The clang-tools package was added by mistake when fixing the failing CI due to the rebase issues. I've removed clang-tools from the install step and CI passes.
Thanks!
3 participants
This PR updates the existing cookie banner consent form (for Google Analytics) for our RTD docs.
This update brings in updated legal terms and UI consistency to match consent banners across Canonical products.
Changes Made
NOTE: The steps to create this PR are outlined in the https://github.com/canonical/RTD-cookie-banner-integration repo README.
In summary I:
Testing Steps
cd docs make clean make install make runOpen the documentation in your browser (use incognito mode or clear cache)
Verify the cookie banner appears as a popup when first visiting the site
Screenshots
Cookie banner on first visit
Configuration choices under "Manage your tracker settings"
Checklist