Refactor h1 and h2 to use Typography component where relevant. #11037
Conversation
|
Build files for cc0abb5 have been deleted. |
|
Size Change: +4.91 kB (+0.22%) Total Size: 2.22 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @benbowler, usage of the Typography component is almost there, but it seems there is an implementation part that has been missed from the IB
Search for <h1 and <h2 across the codebase and replace it by using Typography component, and passing as prop with value of h1 and h2 respectively
Mainly the "passing as prop with value of h1 and h2 respectively". We should use as prop to mark the heading tags correctly, for h1 would be <Typography as="h1" {...}>{...}</Typography> etc.
You can see example of a full usage in the stories file
site-kit-wp/assets/js/components/Typography.stories.js
Lines 52 to 54 in 7d0a6a2
Let's include the missing prop to the implemented Typography components
As to the regard of not replacing headings with Typography in the stories and tests, not sure there is a specific reason we would omit it there, as referenced above, this is already implemented in the Typography.stories.js file. We should replace usage of hardcoded headings completely with the Typography file in the JS files (components, jest tests, storybook stories)
…replace-headings-with-typography-component.
There was a problem hiding this comment.
Thanks @benbowler looks much better. Just one more thing to check
…replace-headings-with-typography-component.
…replace-headings-with-typography-component.
…replace-headings-with-typography-component.
…replace-headings-with-typography-component.
|
Note that E2E tests started failing consistently here, not sure why because the changes are unrelated, however I added timeouts to allow the |
…replace-headings-with-typography-component.
| <h2 className="googlesitekit-heading-3 googlesitekit-setup-module__title"> | ||
| <Typography | ||
| as="h3" | ||
| className="googlesitekit-setup-module__title" | ||
| size="small" | ||
| type="headline" | ||
| > |
There was a problem hiding this comment.
Why the change in heading hierarchy here? This used to be an <h2>.
There was a problem hiding this comment.
This is because it was an h2 with the class googlesitekit-heading-3 - I suppose we could keep as="h2" but the other props as "small" / "headine" to style like it should be. Does that make more sense?
There were a few like this in the codebase where h2s were styled as h3s.
| @@ -13,7 +13,7 @@ exports[`AdminMenuTooltip should render when isTooltipVisible is true and track | |||
| class="googlesitekit-tooltip-body" | |||
| > | |||
| <h2 | |||
| class="googlesitekit-tooltip-title" | |||
| class="googlesitekit-typography googlesitekit-tooltip-title googlesitekit-typography--title googlesitekit-typography--medium" | |||
| > | |||
| Test Title | |||
| </h2> | |||
There was a problem hiding this comment.
Why not use the <Typography> component here too? It would allow us to better test (with snapshots) the output of the component, and might as well use it everywhere we would in production 🤔
There was a problem hiding this comment.
This is the snapshot of the Joyride > Step component which does use the Typography component:
site-kit-wp/assets/js/components/Stepper/Step.js
Lines 33 to 40 in 049b702
There was a problem hiding this comment.
OMG 🤦🏻
My bad for missing that 😅
| <h2 className="googlesitekit-heading-3 googlesitekit-setup-module__title"> | ||
| <Typography | ||
| as="h3" |
There was a problem hiding this comment.
Why the change here to H3? 🤔
…replace-headings-with-typography-component.
Summary
Addresses issue:
h1andh2headings with newTypographycomponent #10905Relevant technical choices
I had to make a few decisions here to refactor title appropriately extending on the IB.
Firstly, I did not refactor the following:
h1
site-kit-wp/assets/js/components/ReportError.stories.js
Line 93 in b21d94c
h2
I found that the following titles used
h2tags but using thegooglesitekit-heading-3class so these should be refactored to theh3style typography component as part of this ticket:assets/js/components/legacy-setup/search-console.jsassets/js/components/legacy-setup/site-verification.jsassets/js/modules/ads/components/setup/SetupMain.jsassets/js/modules/ads/components/setup/SetupMainPAX.jsassets/js/modules/adsense/components/common/AdSenseConnectCTA/Content.jsassets/js/modules/adsense/components/setup/SetupMain.jsassets/js/modules/analytics-4/components/setup/SetupMain.jsassets/js/modules/reader-revenue-manager/components/setup/SetupMain.jsassets/js/modules/sign-in-with-google/components/setup/SetupMain.jsI did this by setting these to
size=smallandas='h3'.Other changes:
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist