Skip to content
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.

Sign up for GitHub

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

Address TODOs #736

Merged
merged 3 commits into from
May 22, 2023
Merged

Address TODOs #736

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0460829
Address TODOs
narcis-radu May 18, 2023
b037415
Merge remote-tracking branch 'upstream/main' into quickfix
narcis-radu May 19, 2023
2aa7809
Merge branch 'main' into quickfix
honstar May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions libs/blocks/global-navigation/features/profile/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,6 @@ class ProfileDropdown {
this.placeholders.manageEnterprise,
this.placeholders.profileAvatar,
],
// TODO: sanity checks if the user is logged in and mandatory properties are set.
// If not, add logs providing guidance for developers
{ displayName: this.profileData.displayName, email: this.profileData.email },
] = await Promise.all([
replaceKeyArray(
Expand Down
2 changes: 0 additions & 2 deletions libs/blocks/global-navigation/features/search/gnav-search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ class Search {
this.isDesktop.addEventListener('change', () => {
closeAllDropdowns();
});

// TODO: search menu should close on scroll, but this should happen from the general Menu logic
}

getSuggestions(query = this.query) {
Expand Down
10 changes: 3 additions & 7 deletions libs/blocks/global-navigation/global-navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,14 +256,11 @@ class Gnav {
};

loadIMS = () => {
const { locale, imsClientId, env } = getConfig();
const { locale, imsClientId, imsScope, env } = getConfig();
if (!imsClientId) return null;
// TODO-1 scopes should be defineable by the consumers
// We didn't have a use-case for that so far
// TODO-2 we should emit an event after the onReady callback
window.adobeid = {
client_id: imsClientId,
scope: 'AdobeID,openid,gnav',
scope: imsScope,
locale: locale?.ietf?.replace('-', '_') || 'en_US',
autoValidateToken: true,
environment: env.ims,
Expand Down Expand Up @@ -337,7 +334,6 @@ class Gnav {
};

decorateAppLauncher = () => {
// TODO: review App Launcher component
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually do need to review this, albeit after MVP. We do have a story for it, so I guess it's ok to remove the comment for now 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is something I mentioned in the description, we can handle the app launcher as part of the upcoming story (post mvp).

// const appLauncherBlock = this.body.querySelector('.app-launcher');
// if (appLauncherBlock) {
// await this.loadDelayed();
Expand Down Expand Up @@ -633,7 +629,7 @@ class Gnav {

export default async function init(header) {
const { locale } = getConfig();
// TODO locale.contentRoot is not the fallback we want
// TODO locale.contentRoot is not the fallback we want if we implement centralized content
const url = getMetadata('gnav-source') || `${locale.contentRoot}/gnav`;
const resp = await fetch(`${url}.plain.html`);
const html = await resp.text();
Expand Down
32 changes: 11 additions & 21 deletions libs/blocks/global-navigation/utilities/getUserEntitlements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,11 @@ import { getConfig } from '../../../utils/utils.js';
const API_WAIT_TIMEOUT = 10000;
const entitlements = {};

const getAcceptLanguage = (locale) => {
let languages = [];
if (!locale || Object.keys(locale).length === 0) {
return languages;
}

languages = [
`${locale.language}-${locale.country.toUpperCase()}`,
`${locale.language};q=0.9`,
'en;q=0.8',
];
return languages;
};
const getAcceptLanguage = (locale = 'en-US') => [
`${locale}`,
`${locale.split('-')[0]};q=0.9`,
'en;q=0.8',
].join(',');

const getQueryParameters = (params) => {
const query = [];
Expand Down Expand Up @@ -94,9 +86,10 @@ const mapSubscriptionCodes = (offers) => {
};
};

const getSubscriptions = async ({ queryParams, locale }) => {
const getSubscriptions = async ({ queryParams }) => {
const config = getConfig();
const profile = await window.adobeIMS.getProfile();
const apiUrl = getConfig().env.name === 'prod'
const apiUrl = config.env.name === 'prod'
? `https://www.adobe.com/aos-api/users/${profile.userId}/subscriptions`
: `https://www.stage.adobe.com/aos-api/users/${profile.userId}/subscriptions`;
const res = await fetch(`${apiUrl}${queryParams}`, {
Expand All @@ -107,7 +100,7 @@ const getSubscriptions = async ({ queryParams, locale }) => {
headers: {
Authorization: `Bearer ${window.adobeIMS.getAccessToken().token}`,
'X-Api-Key': window.adobeIMS.adobeIdData.client_id,
'Accept-Language': getAcceptLanguage(locale).join(','),
'Accept-Language': getAcceptLanguage(config.locale.ietf),
},
})
.then((response) => (response.status === 200 ? response.json() : emptyEntitlements));
Expand All @@ -118,12 +111,10 @@ const getSubscriptions = async ({ queryParams, locale }) => {
* @description Return the JIL User Entitlements
* @param {object} object required params
* @param {array} object.params array of name value query parameters [{name: 'Q', value: 'PARAM'}]
* @param {object} object.locale {country: 'CH', language: 'de'}
* @param {string} object.format format function, raw or default
* @returns {object} JIL Entitlements
*/
// TODO the locale could use the milo format, currently that's an AEM relic.
const getUserEntitlements = async ({ params, locale, format } = {}) => {
const getUserEntitlements = async ({ params, format } = {}) => {
if (!window.adobeIMS?.isSignedInUser()) return Promise.resolve(emptyEntitlements);

const queryParams = getQueryParameters(params);
Expand All @@ -137,8 +128,7 @@ const getUserEntitlements = async ({ params, locale, format } = {}) => {

entitlements[queryParams] = entitlements[queryParams]
|| new Promise((resolve) => {
// TODO we might need to format data for analytics
getSubscriptions({ queryParams, locale })
getSubscriptions({ queryParams })
.then((data) => resolve(data))
.catch(() => resolve(emptyEntitlements));
});
Expand Down
1 change: 0 additions & 1 deletion libs/blocks/global-navigation/utilities/keyboard/mainNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ class MainNavItem {
closeAllDropdowns();
break;
}
// TODO popup navigation logic.
case 'ArrowLeft': {
if (document.dir !== 'rtl') {
if (this.prev === -1) break;
Expand Down
31 changes: 31 additions & 0 deletions libs/blocks/global-navigation/utilities/onImsReady.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
let ready = false;
let existingCall = null;
const onInstance = 'onImsLibInstance';

const onImsReady = (timeout = 3000) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this imported / called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently not used anywhere, but we want to avoid using adobeid.onReady callback given the async nature of blocks. The utility can be used for personalisation, API calls that need user information, tokens etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 For getImsLibInstance I think you should preface it with milo:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event is triggered to allow communication with imslib and get the instance; the name should not be changed.

existingCall = existingCall || new Promise((resolve, reject) => {
const onFail = () => {
if (!ready) reject();
};
const waitTimeout = setTimeout(onFail, timeout);
const onSuccess = (data) => {
const instance = data?.detail?.instance;
if (!instance) {
reject();
return;
}

ready = true;
window.removeEventListener(onInstance, onSuccess);
clearTimeout(waitTimeout);
resolve(instance);
};

window.addEventListener(onInstance, onSuccess);
window.dispatchEvent(new window.CustomEvent('getImsLibInstance'));
overmyheadandbody marked this conversation as resolved.
Show resolved Hide resolved
});

return existingCall;
};

export default onImsReady;
2 changes: 1 addition & 1 deletion libs/blocks/global-navigation/utilities/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export function closeAllDropdowns({ e } = {}) {
el.setAttribute('daa-lh', 'header|Open');
}
});
// TODO the curtain will be refactored

document.querySelector(selectors.curtain)?.classList.remove('is-open');
}

Expand Down
1 change: 1 addition & 0 deletions libs/scripts/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ const config = {
fallbackRouting: 'on',
links: 'on',
imsClientId: 'milo',
imsScope: 'AdobeID,openid,gnav',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a better place to put this stuff. In five years, the config object is going to be huge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't help with size, but we could use nested obj to organize it better.

codeRoot: '/libs',
locales,
prodDomains,
Expand Down