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.

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

Use Brand with Script Link component on services with variants #4948

Merged
merged 52 commits into from
Jan 8, 2020

Conversation

Bopchy
Copy link

@Bopchy Bopchy commented Dec 11, 2019

Resolves #4009

Overall change:
Use Brand with ScriptLink component on services with variants.

Code changes:

  • Update Brand version number.
  • Add ScriptLink and SkipLink components to Brand.
  • Update existing snapshots and tests.
  • Update config to include service variants, script link text and script link offscreen text for UKChina, Serbian and Zhongwen.
  • Add unit tests.
  • Add onClick handler that sets the variant preference cookie when the user interacts with the link.
  • Add setPreferredVariantCookie() to UserContext.

  • I have assigned myself to this PR and the corresponding issues
  • I have added labels to this PR for the relevant pod(s) affected by these changes
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)
  • This PR requires manual testing

@PriyaKR PriyaKR self-assigned this Dec 31, 2019
@PriyaKR
Copy link
Contributor

PriyaKR commented Dec 31, 2019

Testing:

Noticed few issues across cross browser and device testing

@Bopchy
Copy link
Author

Bopchy commented Dec 31, 2019

On desktop browsers when on http://localhost:7080/ukchina/trad and then click one of the links on the top nav and then click the brand svg it gets redirected to http://localhost:7080/ukchina/trad but for a short time http://localhost:7080/ukchina/simp url is seen should be resolved by #4976.

Seeing http://localhost:7080/ukchina/simp for a short time before it redirects to http://localhost:7080/ukchina/trad is being caused by the default variant being accessed before the cookie redirect happens

@Bopchy
Copy link
Author

Bopchy commented Jan 1, 2020

Regarding Also on IE9 the service variant button appears to be in wrong place just below the brand svg not an issue on test env., the service variant button appears just below the brand svg only on small screen widths on IE, which is in-line with the designs

Screenshot 2020-01-01 at 23 09 17

@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 2, 2020

@Bopchy i see the IE9 issue happening on wider screens.

@Bopchy
Copy link
Author

Bopchy commented Jan 2, 2020

The IE9 issue (pictured below) is happening because IE9 doesn't fully support display: flex which has been used to style the div.SvgWrapper, which contains all the <Banner /> links. As such the contents of display: flex are not properly positioned out of the box. This has been documented here.

Screenshot 2020-01-03 at 00 00 25

Will not be fixing this though as IE9 is not on the list of supported browsers

@Bopchy
Copy link
Author

Bopchy commented Jan 6, 2020

On mobile browsers when on http://localhost:7080/ukchina/trad and then click one of the links on the top nav and then click the brand svg it gets redirected to http://localhost:7080/ukchina/simp and not to http://localhost:7080/ukchina/trad is happening because the preferred variant cookie is not being set on mobile - so clicking on the brand svg redirects to the default variant

This should be fixed in #5038

@Bopchy
Copy link
Author

Bopchy commented Jan 7, 2020

Travis failures will be fixed in https://github.com/bbc/simorgh/tree/integrate-new-navigation once this work is merged, as the same failures are happening in parent branch as well

@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 7, 2020

@Bopchy I am now seeing the mobile issue mentioned above on desktop as well.May be we should tweak the issue saying Cookie redirects do not work on mobile and desktop

@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 7, 2020

I have updated the issue #5038 to include the desktop issue as well.So this PR can be merged now as separate issue is being created for the bugs mentioned above.

@tochwill tochwill merged commit b5c2995 into integrate-new-navigation Jan 8, 2020
@tochwill tochwill deleted the 4009-use-brand-with-script-link branch January 8, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants