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.

Sign up for GitHub

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

Jump to bottom

fix(content-group): adding h-tags to headings #11738

Merged
merged 16 commits into from
May 8, 2024
Merged

fix(content-group): adding h-tags to headings #11738

merged 16 commits into from
May 8, 2024

Conversation

marcelojcs
Copy link
Contributor

Related Ticket(s)

Jira

The H3 and H4 headings are not properly configured in the AEM template. They should be tagged as the H1 and H2 (from the same AEM template). Otherwise, they won't have the same SEO relevancy because it's not recognized as H2 & H3.

Description

Adding the respective H tags to content-group-heading and content-item-heading components.

@marcelojcs marcelojcs requested a review from a team as a code owner April 23, 2024 14:08
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 23, 2024 •
edited
Loading

Deploy preview created for package Carbon Web Components:
https://carbon-web-components.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 23, 2024 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 23, 2024 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 23, 2024 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

@marcelojcs marcelojcs mentioned this pull request Apr 23, 2024
Open
@marcelojcs
Copy link
Contributor Author

V2 version of this PR

#11739

@andy-blum andy-blum added the test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing label Apr 24, 2024
@andy-blum andy-blum closed this Apr 24, 2024
@andy-blum andy-blum reopened this Apr 24, 2024
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 24, 2024 •
edited
Loading

Deploy preview created for package Deploy Preview CDN:
https://ibmdotcom-cdn.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 9071a87f51e4ff6e05c5e96804ca5bd7c6cca4df

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Apr 25, 2024 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

@andy-blum
Copy link
Member

It seems that in addition to adding the h3/h4 elements to the shadowroot, we'll need to remove the role and aria-level properties in use here. As it is now, the headings appear twice in screen readers.

Screenshot 2024-04-26 at 2 29 13 PM

@andy-blum andy-blum added the Needs design approval PRs on feature requests and new components have to get design approval before merge. label Apr 26, 2024
@andy-blum
Copy link
Member

@marcelojcs looks like you need to update the unit test snapshots.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 3, 2024 •
edited
Loading

Deploy preview created for package IBM.com Web Components Deploy Preview CDN:
https://ibmdotcom-cdn.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/11738/index.html

Built with commit: 6d18c27c8ac400ecd5b7e6e6e951c9cc2af0ece7

jkaeser
jkaeser approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

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

Code changes here look good to me!

@andy-blum
Copy link
Member

There are some subtle but noticable differences in the typography with the new heading elements. Can we get some designer input on what this is supposed to be rendering as?

Screenshot 2024-05-06 at 4 26 57 PM Screenshot 2024-05-06 at 4 27 37 PM

@RichKummer
Copy link

Hey @marcelojcs , I'm seeing the heading for content item rendering differently than it should. It should use the fixed$heading-02 token for the heading in the v1 package.

This PR:
Screenshot 2024-05-06 at 4 27 39 PM

Spec samples:
Screenshot 2024-05-06 at 4 34 10 PM

@jkaeser
Copy link
Member

jkaeser commented May 7, 2024

@marcelojcs I think ci-check is failing on style linting. Can you run yarn format from the repo root and commit the changes it makes?

@RichKummer
Copy link

Hey @marcelojcs and @andy-blum ,

content item looks fixed. I noticed that for content group, the heading does not appear to be updating its size as $expressive-heading-04 across breakpoints. It should be 28px up to @lg and then update to 32px for @lg – @max.

Right now, I only see the heading render as 28px across breakpoints.

@marcelojcs
Copy link
Contributor Author

@RichKummer
Addressed in my latest commit.

@RichKummer
Copy link

Thanks @marcelojcs ! Both content item and group look fixed. I did see that for some reason the content item horizontal's header updated to a different type token than it should.

It should appear with the heading as $expressive-heading-03

Screenshot 2024-05-08 at 1 29 12 PM

@andy-blum
Copy link
Member

@RichKummer updated font rules to inherit their font letter-spacing and line-height rules from the parent components.

RichKummer
RichKummer approved these changes May 8, 2024
View reviewed changes
Copy link

@RichKummer RichKummer left a comment

Choose a reason for hiding this comment

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

@marcelojcs @andy-blum Thanks for the fixes! LGTM

@ariellalgilmore ariellalgilmore merged commit 8d61228 into carbon-design-system:v1 May 8, 2024
22 of 24 checks passed
@andy-blum andy-blum added Ready to merge Label for the pull requests that are ready to merge and removed Needs design approval PRs on feature requests and new components have to get design approval before merge. labels May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkaeser jkaeser jkaeser approved these changes

@RichKummer RichKummer RichKummer approved these changes

@andy-blum andy-blum andy-blum approved these changes

@IgnacioBecerra IgnacioBecerra Awaiting requested review from IgnacioBecerra IgnacioBecerra is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@annawen1 annawen1 Awaiting requested review from annawen1 annawen1 is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

Assignees
No one assigned
Labels
Ready to merge Label for the pull requests that are ready to merge test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@marcelojcs @ibmdotcom-bot @andy-blum @RichKummer @jkaeser @ariellalgilmore