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

Improve tag name determination #1989

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

swissspidy
Copy link
Collaborator

Tries to ensure that there’s only ever one h1 tag on a page.

Also takes into account elements positioned at the very bottom of a page. These are unlikely to be headings and will be wrapped in <p> tags.

Fixes #1881.

Tries to ensure that there’s only ever one h1 tag on a page
@googlebot googlebot added the cla: yes Signed the Google CLA label Mar 18, 2019
@miina
Copy link
Contributor

miina commented Mar 18, 2019

Maybe we could also reflect the tag switch within the editor by changing the font weight?

@swissspidy
Copy link
Collaborator Author

Maybe we could also reflect the tag switch within the editor by changing the font weight?

Sounds interesting, although I am not sure if the changes would be a bit unexpected for users. Perhaps worth exploring in a future PR?

To give an example of why it might be unexpected:

Let's say we have two blocks, A (content: "Foo") and B (content: "Bar"). Because of the maybeSetTagName() logic, A gets an h1 tag, and B an h2. When I now enter some longer text in A, it gets degraded to a p and B gets promoted to a h1. -> The font weight changes for both.

@miina
Copy link
Contributor

miina commented Mar 18, 2019

Sounds interesting, although I am not sure if the changes would be a bit unexpected for users. Perhaps worth exploring in a future PR?

Yes, probably it would be unexpected. I was thinking of this due to the difference between the frontend and the editor view -- probably what the user sees in the frontend is also unexpected (bold in the frontend, normal weight in the editor). Alternatively we could by default remove the bold font-weight from headings in the frontend, however, that might not be the best either. Thoughts?

We can definitely leave this for a future PR, possibly as part of #1967.

@swissspidy
Copy link
Collaborator Author

Alternatively we could by default remove the bold font-weight from headings in the frontend, however, that might not be the best either.

That would be my suggestion for now though. The important part is the semantic meaning. Boldness can always be added via the block toolbar.

Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

LGTM!

@swissspidy swissspidy merged commit 3c8368c into amp-stories-redux Mar 18, 2019
@swissspidy swissspidy deleted the amp-story/1881-improve-semantics branch March 18, 2019 21:33
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants