Skip to content

Updating example template #95

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

Merged
merged 2 commits into from
Apr 14, 2025
Merged

Updating example template #95

merged 2 commits into from
Apr 14, 2025

Conversation

aldimar-junior
Copy link
Contributor

@aldimar-junior aldimar-junior commented Apr 8, 2025

If there's more than one example appends a <h3> tag with the number of the example, i.e:

"Example Nº 1"

Related to/Fixes api.jquery.com issue #1157

If there's more than one example appends a <h3> tag with the number of the example, i.e:

"Example Nº 1"
@mgol
Copy link
Member

mgol commented Apr 8, 2025

Thanks for the PR. This is what it looks like at the beginning of the examples section:
Screenshot 2025-04-08 at 23 23 35
and between examples:
Screenshot 2025-04-08 at 23 22 50

A few remarks:

  1. The "Nº" part is a bit much to my eye, I'd leave it at Example 1.
  2. There's no margin above the header; this is a result of base.css styles. I think this was done as usually the header follows a paragraph which has a bottom margin but in this case we have a code block at the end of the previous example so it seems a bit cramped (see the screenshot above). I'd add a style to base.css in jquery-wp-content for .entry-example h3, setting margin-top to 1em. I propose setting it on the h3, not on the entry itself so that it's not set if we only have 1 example and the header is skipped.

See the screenshots below for what it'd look like.

The beginning of the examples section:
Screenshot 2025-04-08 at 23 29 18
and between examples:
Screenshot 2025-04-08 at 23 29 24

@timmywil @Krinkle what do you think? This will affect all content sites with examples.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

See my suggestions, but I'd also like to see what others think.

@aldimar-junior
Copy link
Contributor Author

@mgol should I go ahead and make the suggested changes or wait for further review?

@mgol
Copy link
Member

mgol commented Apr 11, 2025

@aldimar-junior as you prefer

@aldimar-junior
Copy link
Contributor Author

aldimar-junior commented Apr 11, 2025

@mgol I made a new PR to jquery-wp-content adding only the style for .entry-example h3 and made a new commit here removing the "Nº" from the example header

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait for another review before merging. Thanks!

@mgol mgol requested a review from timmywil April 12, 2025 11:33
mgol pushed a commit to jquery/jquery-wp-content that referenced this pull request Apr 14, 2025
@mgol mgol merged commit 00ee37d into jquery:main Apr 14, 2025
3 checks passed
@mgol
Copy link
Member

mgol commented Apr 14, 2025

Timmy approved during the jQuery Core team meeting so I've merged this. Thanks!

mgol added a commit to jquery/api.jquery.com that referenced this pull request Apr 14, 2025
@mgol mgol removed the Needs review label Apr 14, 2025
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.

2 participants