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
Contributor

@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

@Bopchy Bopchy added this to PR in Progress in Simorgh via automation Dec 12, 2019
@Bopchy Bopchy moved this from PR in Progress to Code review in Simorgh Dec 12, 2019
@Bopchy Bopchy moved this from Code review to PR in Progress in Simorgh Dec 13, 2019
@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

@PriyaKR PriyaKR moved this from In Test to PR in Progress in Simorgh Dec 31, 2019
@Bopchy
Copy link
Contributor 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
Contributor 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
Contributor 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
Contributor 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 Bopchy moved this from PR in Progress to Ready for Test in Simorgh Jan 7, 2020
@Bopchy
Copy link
Contributor 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 PriyaKR moved this from Ready for Test to In Test in Simorgh Jan 7, 2020
@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.

@PriyaKR PriyaKR moved this from In Test to Ready for merge (probably) in Simorgh Jan 7, 2020
@AlistairGempf AlistairGempf mentioned this pull request Jan 8, 2020
6 tasks
@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
Simorgh automation moved this from Ready for merge (probably) to Done Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants