Head and meta tags handling #1537

revanth0212 · 2019-08-07T19:40:29Z

Added wrappers around the HeadProvider and Title components of react-head. Also exporting Meta, Link and Style tags directly from react-head.

These 5 components will be used to handle the document's head tag.

To be able to use the components anywhere in the app, the app has to be wrapped inside HeadProvider which provides the context needed to maintain all the rendered tags.

Closes 422

vercel · 2019-08-07T19:40:35Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://pwa-studio-git-headandmetatagshadling.mmansoor.now.sh

PWAStudioBot · 2019-08-07T19:46:49Z

	Fails
🚫	Missing "Description" section. Please add it back, with detail.
🚫	Missing "Verification Steps" section. Please add it back, with detail.
🚫	No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" in your PR.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section.

Generated by 🚫 dangerJS against 75d9637

packages/venia-concept/src/components/Head/headProvider.js

packages/venia-concept/src/components/Head/title.js

packages/venia-concept/package.json

supernova-at

Some minor changes requested. I'd love to be able to remove the custom <title> filtering if we can. That'll let us use the components directly from react-head, which means less code for us to maintain 😉

supernova-at · 2019-08-08T19:51:56Z

packages/venia-concept/src/components/Head/__tests__/headProvider.spec.js

+
+afterEach(cleanup);
+
+test('snapshot testing HeadProvider', () => {


The rest of the tests have nice descriptive names for what they are testing, for example "HeadProvider should replace all previous title tags with the latest tag".

Can you provide a nice descriptive name for this test as well?

Not sure what to call this. It is a casual snapshot test.

"HeadProvider by itself doesn't render anything" ?

Basically you just want someone who is only looking at the snapshot to be able to ensure it is correct.

packages/venia-concept/src/components/Head/__tests__/headProvider.spec.js

packages/venia-concept/src/components/Head/headProvider.js

supernova-at · 2019-08-08T20:14:11Z

packages/venia-concept/src/components/Head/headProvider.js

@@ -0,0 +1,19 @@
+import React, { useEffect } from 'react';
+import { HeadProvider as _HeadProvider } from 'react-head';


I'm not a huge fan of the underscore as the first character of a React component 😬

What shall I change it to? I wanted to signify vanila HeadProvider as _HeadProvider.

Yeah I wasn't really sure either 😝

What if we called our function VeniaHeadProvider, and it rendered react-head's HeadProvider.

Because VeniaHeadProvider would be the default export, you could still import it from other files as HeadProvider.

Interesting. Let me get this done.

@supernova-at The pattern of naming things VeniaHeadProvider after the package is kind of frustrating in practice. These components get used in implementations that aren't identified as Venia anymore and that naming doesn't feel good to see, so we rewrite and maintain that rewrite. It makes upstream integration more complex.

packages/venia-concept/src/components/Head/title.js

supernova-at · 2019-08-08T20:28:42Z

#1537 (comment). Please fill out the PR template :)

Looks like there are some conflicts to resolve as well 👍

supernova-at · 2019-08-09T14:37:01Z

I also have a small concern about stale meta tags hanging around.

If Page2 provides the same meta tag as Page1 with a different value, it'll get overwritten properly. But what if Page1 has a specific meta tag that Page2 doesn't care / know about? Won't that just stick around forever?

revanth0212 · 2019-08-09T15:42:18Z

I also have a small concern about stale meta tags hanging around.

If Page2 provides the same meta tag as Page1 with a different value, it'll get overwritten properly. But what if Page1 has a specific meta tag that Page2 doesn't care / know about? Won't that just stick around forever?

If the new page is a new HTML it shouldn't unless the new markup has that meta tag. If it a page transition using react-router, then it is on us to clear those tags. As of today, react-head does not support the manipulations of tags (like removing). I have checked their codebase, it is possible for us to go ahead and add manipulations API as submit a PR, or the simplest would be to write a simple wrapper around Meta (just like how we did for Title) that does these kinds of manipulations.

The ticket dint mention those use cases but you are right, they will be hanging around and there has to be a way to delete the existing ones. Shall I go ahead and write that wrapper? What do you say @jimbo @supernova-at?

…ling

dpatil-magento · 2019-08-16T17:29:11Z

@revanth0212 Please check unit test failures. Able to reproduce on my local as well.
packages/venia-ui/lib/components/App/tests/app.spec.js
packages/venia-ui/lib/RootComponents/Category/tests/categoryContent.spec.js

Also If I visit any page for the first time then title shows null - Venia for fraction of time.

jimbo

This works well. We can iterate on the specific title format in the future.

jimbo · 2019-08-16T21:17:23Z

This is ready to go pending QA approval.

revanth0212 added the pkg:peregrine label Aug 7, 2019

revanth0212 requested a review from jimbo August 7, 2019 19:40

revanth0212 self-assigned this Aug 7, 2019

PWAStudioBot added the pkg:venia-concept label Aug 7, 2019

brendanfalkowski reviewed Aug 7, 2019

View reviewed changes

packages/venia-concept/src/components/Head/headProvider.js Outdated Show resolved Hide resolved

brendanfalkowski reviewed Aug 7, 2019

View reviewed changes

packages/venia-concept/src/components/Head/title.js Outdated Show resolved Hide resolved

VladimirZaets added this to Work in Progress in Pull Request Progress Aug 8, 2019

m2-community-project bot moved this from Work in Progress to Review in Progress in Pull Request Progress Aug 8, 2019

m2-community-project bot added the Progress: review label Aug 8, 2019

supernova-at assigned supernova-at and unassigned revanth0212 Aug 8, 2019

vercel bot deployed to staging August 8, 2019 20:09 View deployment

m2-community-project bot assigned revanth0212 Aug 8, 2019

revanth0212 commented Aug 8, 2019

View reviewed changes

packages/venia-concept/package.json Outdated Show resolved Hide resolved

supernova-at requested changes Aug 8, 2019

View reviewed changes

vercel bot deployed to staging August 9, 2019 15:57 View deployment

PWAStudioBot added pkg:babel-preset-peregrine pkg:graphql-cli-validate-magento-pwa-queries pkg:pwa-buildpack pkg:upward-js Pertains to upward-js reference implementation of UPWARD. pkg:upward-spec Pertains to UPWARD specification package. labels Aug 9, 2019

vercel bot deployed to staging August 9, 2019 16:18 View deployment

revanth0212 added 3 commits August 9, 2019 11:34

Created required tags.

89fcdbe

Added tests.

847c2cd

Using syntetictag instead of synteticTag.

f0c6bac

Merge remote-tracking branch 'origin/develop' into headAndMetaTagsHad…

fbd90c8

…ling

vercel bot deployed to staging August 16, 2019 14:57 View deployment

zetlen changed the title ~~Head and meta tags hadling~~ Head and meta tags handling Aug 16, 2019

m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Aug 16, 2019

m2-community-project bot added Progress: approved and removed Progress: review labels Aug 16, 2019

dpatil-magento moved this from Reviewer Approved to QA In Progress in Pull Request Progress Aug 16, 2019

m2-community-project bot removed the Progress: approved label Aug 16, 2019

Merge branch 'develop' into headAndMetaTagsHadling

8b901fa

vercel bot deployed to staging August 16, 2019 17:04 View deployment

Snapshop update.

acd2a69

revanth0212 dismissed supernova-at’s stale review via acd2a69 August 16, 2019 17:39

vercel bot deployed to staging August 16, 2019 17:39 View deployment

revanth0212 added 2 commits August 16, 2019 15:02

Not showing category title while loading.

4e88728

Snapshot update.

9ce1fae

vercel bot temporarily deployed to staging August 16, 2019 20:03 Inactive

Reverting minor changes.

3e0dacd

vercel bot deployed to staging August 16, 2019 20:06 View deployment

Snapshot update.

040d4d1

vercel bot deployed to staging August 16, 2019 20:08 View deployment

Mock react-head components

069c9c7

vercel bot deployed to staging August 16, 2019 20:31 View deployment

Fix App snapshot

75d9637

vercel bot deployed to staging August 16, 2019 21:11 View deployment

jimbo approved these changes Aug 16, 2019

View reviewed changes

dpatil-magento merged commit 0186e9a into develop Aug 16, 2019

dpatil-magento deleted the headAndMetaTagsHadling branch August 16, 2019 21:40

m2-community-project bot moved this from QA In Progress to Done in Pull Request Progress Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Head and meta tags handling #1537

Head and meta tags handling #1537

revanth0212 commented Aug 7, 2019

vercel bot commented Aug 7, 2019 •

edited

PWAStudioBot commented Aug 7, 2019 •

edited

supernova-at left a comment

supernova-at Aug 8, 2019

revanth0212 Aug 9, 2019

supernova-at Aug 9, 2019

supernova-at Aug 8, 2019

revanth0212 Aug 9, 2019

supernova-at Aug 9, 2019

revanth0212 Aug 12, 2019

brendanfalkowski Aug 12, 2019

supernova-at commented Aug 8, 2019

supernova-at commented Aug 9, 2019

revanth0212 commented Aug 9, 2019

dpatil-magento commented Aug 16, 2019 •

edited

jimbo left a comment

jimbo commented Aug 16, 2019


		afterEach(cleanup);

		test('snapshot testing HeadProvider', () => {

		@@ -0,0 +1,19 @@
		import React, { useEffect } from 'react';
		import { HeadProvider as _HeadProvider } from 'react-head';

Head and meta tags handling #1537

Head and meta tags handling #1537

Conversation

revanth0212 commented Aug 7, 2019

vercel bot commented Aug 7, 2019 • edited

PWAStudioBot commented Aug 7, 2019 • edited

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at commented Aug 8, 2019

supernova-at commented Aug 9, 2019

revanth0212 commented Aug 9, 2019

dpatil-magento commented Aug 16, 2019 • edited

jimbo left a comment

Choose a reason for hiding this comment

jimbo commented Aug 16, 2019

vercel bot commented Aug 7, 2019 •

edited

PWAStudioBot commented Aug 7, 2019 •

edited

dpatil-magento commented Aug 16, 2019 •

edited