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

Jump to bottom

[MWPW-128550] Profile block fixes #581

Merged
merged 2 commits into from
Mar 21, 2023
Merged

[MWPW-128550] Profile block fixes #581

merged 2 commits into from
Mar 21, 2023

Conversation

overmyheadandbody
Copy link
Contributor

@overmyheadandbody overmyheadandbody commented Mar 21, 2023 •
edited
Loading

This PR handles a few bugs which were found in the Profile block logic:

  • splits logic in a slightly different way to only load what is required in any given scenario:
    • if the user is signed out, load the logic that adds the Sign In workflow, either simple or with a dropdown;
    • if the user is signed in, load the logic that shows the Profile trigger. Once clicked or after 3s have passed, the logic that handles the dropdown will also be loaded and executed;
  • ensure the language is attached to the 'View Account' link;
  • ensure dropdown is opened when the Profile avatar is clicked;
  • fetch IMS and placeholders concurrently as a micro-optimisation;
  • remove previous logic with gnav classes;
  • attach styles to links from the Profile dropdown as opposed to their wrapping li elements;
  • go for more consistent styling when it comes to the Profile dropdown links;
  • start having a more consistent approach when it comes to exporting/importing modules, sometimes we expose some methods, either directly or as part of an object, sometimes we expose whole objects, this should be cleaned up as we go;
  • separate App Launcher from Profile;
  • small bonus 1: add box shadow to navigation dropdown menus;
  • small bonus 2: ensure proper hamburger menu alignment on RTL locales;

Please note that the standalone implementation POC has not been updated, since it's not a priority to implement this right away.

Resolves: MWPW-128550

Test URLs:

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Mar 21, 2023 •
edited
Loading

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Mar 21, 2023

Page Score PSI Audit Google
/drafts/ramuntea/gnav-refactor?martech=off SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Mar 21, 2023

Page Score PSI Audit Google
/drafts/ramuntea/gnav-refactor?martech=off SI FCP LCP TBT CLS PSI

narcis-radu
narcis-radu approved these changes Mar 21, 2023
View reviewed changes
libs/blocks/global-navigation/global-navigation.js
const accessToken = window.adobeIMS.getAccessToken();
const { env } = getConfig();
const headers = new Headers({ Authorization: `Bearer ${accessToken.token}` });
const profileData = await fetch(`https://${env.adobeIO}/profile`, { headers });
Copy link
Contributor

Choose a reason for hiding this comment

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

is adobeIO used anywhere else? Maybe we can rename to profileAPI or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place where we use it so far. Milo exposes the hostname in their getConfig() method as part of the env property and its value reflects the current environment. If it will be needed in any other place, we can save it somewhere, but it's not the case right now.

libs/blocks/global-navigation/global-navigation.css
@@ -76,6 +76,11 @@
background-color: var(--feds-background-nav--light);
}

[dir = "rtl"] .feds-nav-wrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick - there's mixed dir='rtl' and dir = "rtl" throughout the code, we should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the two instances where we were using single quotes and no spaces - 00cdf22

libs/blocks/global-navigation/blocks/profile/signIn.js
dropdownElem.classList.add('feds-signIn-dropdown');

// TODO we don't have a good way of adding attributes to links
const dropdownSignIn = dropdownElem.querySelector('[href="https://adobe.com?sign-in=true"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add another TODO here to revisit the logic, it doesn't make sense to show the URL for non adobe.com pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue that if a consumer doesn't want the IMS login option, they shouldn't add the https://adobe.com?sign-in=true link in their navigation document. We're not adding it by default, only if that link is authored.

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented Mar 21, 2023

Page Score PSI Audit Google
/drafts/ramuntea/gnav-refactor?martech=off SI FCP LCP TBT CLS PSI

@overmyheadandbody overmyheadandbody merged commit 4607b16 into adobecom:gnav Mar 21, 2023
@overmyheadandbody overmyheadandbody deleted the MWPW-128550-profile-block-fixes branch March 21, 2023 16:00
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Mar 28, 2023
@overmyheadandbody
[MWPW-128550] Profile block fixes (adobecom#581)
4b6a7a5 
* [MWPW-128550] Profile block fixes

* [MWPW-128550] Profile block fixes PR feedback implementation

---------

Co-authored-by: Rares Munteanu <ramuntea@adobe.com>
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Mar 29, 2023
@overmyheadandbody
[MWPW-128550] Profile block fixes (adobecom#581)
0d6e85e 
* [MWPW-128550] Profile block fixes

* [MWPW-128550] Profile block fixes PR feedback implementation

---------

Co-authored-by: Rares Munteanu <ramuntea@adobe.com>
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Mar 31, 2023
@overmyheadandbody
[MWPW-128550] Profile block fixes (adobecom#581)
7b0dce9 
* [MWPW-128550] Profile block fixes

* [MWPW-128550] Profile block fixes PR feedback implementation

---------

Co-authored-by: Rares Munteanu <ramuntea@adobe.com>
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Apr 4, 2023
@overmyheadandbody
[MWPW-128550] Profile block fixes (adobecom#581)
6e8513e 
* [MWPW-128550] Profile block fixes

* [MWPW-128550] Profile block fixes PR feedback implementation

---------

Co-authored-by: Rares Munteanu <ramuntea@adobe.com>
overmyheadandbody added a commit that referenced this pull request Apr 13, 2023
@overmyheadandbody
[MWPW-128550] Profile block fixes (#581)
faadfac 
* [MWPW-128550] Profile block fixes

* [MWPW-128550] Profile block fixes PR feedback implementation

---------

Co-authored-by: Rares Munteanu <ramuntea@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narcis-radu narcis-radu narcis-radu approved these changes

@mokimo mokimo Awaiting requested review from mokimo

Assignees

@narcis-radu narcis-radu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@overmyheadandbody @narcis-radu