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

Display warning on preview environment to notify users if page is Mozart #12038

Open
wants to merge 21 commits into
base: latest
Choose a base branch
from

Conversation

karinathomasbbc
Copy link
Contributor

@karinathomasbbc karinathomasbbc commented Oct 9, 2024

Resolves JIRA N/A

Overall changes

Displays a warning on the preview environment if a page is rendered by Mozart (meaning that it cannot pick up changes from branch/PR)

Code changes

Add an indicator component to the header/footer
Add requestServiceChain to the request context

Screenshot 2024-10-11 at 12 31 00
Screenshot 2024-10-11 at 12 31 10

Testing

Locally - cannot test on preview for pages not routed directy to Simorgh as they are rendering the test.bbc.com version
Unit tests
Preview environment: indicator not displayed for pages routed directly to Simorgh

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

@karinathomasbbc karinathomasbbc self-assigned this Oct 9, 2024
@karinathomasbbc karinathomasbbc changed the title WIP: Display banner on preview environment to notify users if page is Mozart Display banner on preview environment to notify users if page is Mozart Oct 9, 2024
@karinathomasbbc karinathomasbbc changed the title Display banner on preview environment to notify users if page is Mozart Display warning on preview environment to notify users if page is Mozart Oct 11, 2024
@karinathomasbbc karinathomasbbc marked this pull request as ready for review October 11, 2024 11:27
Copy link
Contributor

@HarveyPeachey HarveyPeachey left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 183 to 192
const [isPreview, setIsPreview] = useState(false);

useEffect(() => {
const hostnameMatchesPreview = window.location.hostname.match(/preview/g);
const isPreviewEnvironment =
hostnameMatchesPreview && hostnameMatchesPreview.length > 0;

setIsPreview(isPreviewEnvironment);
}, []);

Copy link
Contributor

Choose a reason for hiding this comment

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

Just need to double check that no CLS or LCP issues happen or any weird behaviour with child components, as using state might cause a re-render.

Not actually sure if react does a re-render when you set an already false state value to false, hopefully it's clever enough, or you could be doubly safe and just set the state when isPreviewEnvironment is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gosh I never thought about LCP/CLS 😬 Luckily this is only on the preview environment, but all the same, it's best to check. I'll ensure not to set state unless preview env is true & see if that causes a re-render or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But of course this code will run regardless of which environment it's on. I'll do a quick comparison between the test environment & preview with this banner to see if there's a difference in Web Vital scores.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be fine, as I don't think there's anything that will actually make react change anything in the DOM, but best to double check

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.

3 participants