-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hds 1854 new cookie consent rebase #1312
Hds 1854 new cookie consent rebase #1312
Conversation
The css is built in another branch
The objects are empty, but sometimes props are needed.
…or sitesettings object, monitoring and removing cookies,localStorage,sessionStorage,indexedDB,cacheStorage,etc
…wing consenting them via api
The paths to files are incorrect
packages/react/src/components/cookieConsentCore/cookieConsentCore.js
Outdated
Show resolved
Hide resolved
pageContentSelector = 'body', // Where to add scroll-margin-bottom | ||
submitEvent = false, // if string, do not reload page, but submit the string as event after consent | ||
settingsPageSelector = null, // If this string is set and a matching element is found on the page, show cookie settings in a page replacing the matched element. | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second argument is an object, which could also include the first argument, so there would only be one. Code is cleaner and it is easier to pick params and set defaults.
// outside constructor
const defaults = {
language: 'en',
theme = 'bus',
...rest
}`
constructor(options){
const parsedOptions = {...defaults, ...options}
const { language, theme, ...rest} = parsedOptions;
}
(The third one is not really needed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this change, there could only be one private property "this.settings" instead of
#LANGUAGE;
#THEME;
#TARGET_SELECTOR;
#SPACER_PARENT_SELECTOR;
#PAGE_CONTENT_SELECTOR;
#SUBMIT_EVENT = false;
#SETTINGS_PAGE_SELECTOR;
And the this.settings
could be passed in function calls
await this.#render(this.#LANGUAGE, siteSettings, false, settingsPageElement);
could be
await this.#render(this.settings, false);
and
const message = getTranslation(
this.#SITE_SETTINGS.translations,
'settingsSaved',
this.#LANGUAGE,
this.#SITE_SETTINGS.fallbackLanguage,
);
could also be simplified.
submitEvent = false, // if string, do not reload page, but submit the string as event after consent | ||
settingsPageSelector = null, // If this string is set and a matching element is found on the page, show cookie settings in a page replacing the matched element. | ||
}, | ||
calledFromCreate = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fail-safe seems overkill. Documentation should be enough, and if not, just do not publish the constructor in the package if it should never be used.
packages/react/src/components/cookieConsentCore/cookieConsentCore.js
Outdated
Show resolved
Hide resolved
* @throws {Error} Throws an error if siteSettingsParam is an URL string and the JSON parsing fails. | ||
*/ | ||
static async create(siteSettingsParam, options) { | ||
let instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If constructor is changed to accept only one argument (options), then same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below for reasoning behind this choise.
packages/react/src/components/cookieConsentCore/cookieConsentCore.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/cookieConsentCore/cookieConsentCore.js
Outdated
Show resolved
Hide resolved
…ndles HDS-2298: New cookie consent standalone build
… for adding to hds scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tiny remarks I had forgotten pending.
font-size: var(--fontsize-heading-xs); | ||
font-weight: bold; | ||
letter-spacing: 0.4px; | ||
line-height: 24px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be suitable line-height-token?
|
||
.hds-notification__body { | ||
font-size: var(--fontsize-body-s); | ||
line-height: 24px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, line-heigth token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
packages/react/src/components/cookieConsentCore/translations.js
Outdated
Show resolved
Hide resolved
…building standalone cookie banner
…and fallback status
…. So that wanted fallback language features does not generate errors and add test for it.
packages/react/src/components/cookieConsentCore/siteSettingsEditor/cookie-banner-admin-ui.js
Show resolved
Hide resolved
…ations-tests (HDS-2430) Add tests for translations.js
Description
Preliminary draft, still things to do:
Related Issue
Closes #
Motivation and Context
How Has This Been Tested?
Demos:
Links to demos are in the comments
Screenshots (if appropriate):
Add to changelog