Nothing Special   »   [go: up one dir, main page]
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

Update validate-h-card #101

Merged
merged 5 commits into from
Sep 30, 2022
Merged

Update validate-h-card #101

merged 5 commits into from
Sep 30, 2022

Conversation

gRegorLove
Copy link
Member
@gRegorLove gRegorLove commented Jun 30, 2022 •
edited
Loading

This resolves #99 and streamlines a few things on the /validate-h-card page:

  • Updated https://github.com/barnabywalters/php-mf-cleaner
  • Use the mf-cleaner's updated getRepresentativeHCard method instead of duplicating the rep h-card code inline
  • Flatten some of the if/else logic in the page, splitting out if ($error)... and if ($showResult)...
  • Use the mf-cleaner's getPlaintextArray to parse multi-value mf2 properties for display
  • Updated microformats.org links to https
  • Added HTML <label> for accessibility, added required attribute
  • Some updates to reduce the amount of PHP notices (when error_reporting is on), using isset and null coalescing operator

You can test these updates on https://iwm.gregoreatworld.com/validate-h-card/

The URL from #99 is working with these updates:
https://iwm.gregoreatworld.com/validate-h-card/?url=https%3A%2F%2Fpresence-gqle9t8ex-johanbove.vercel.app%2F

Edit: contextualized the message when there's no rep h-card, but one or more h-cards:

  • multiple h-cards: "Multiple h-cards were found on your site! Consider only having one and marking it up..." Example
  • one h-card: "An h-card was found on your site! Consider marking it up..." Example

jalcine
jalcine approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor
@jalcine jalcine left a comment

Choose a reason for hiding this comment

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

Nice typographical updates as well as cleaning up some logic!

@gRegorLove
Copy link
Member Author

I need to push a small change before this is ready. The second condition (no rep h-card, 1 or more h-cards found) is always saying "Multiple h-cards were found...". I'll make it say "An h-card was found..." if there's only one.

@gRegorLove gRegorLove requested a review from aaronpk June 30, 2022 21:05
tantek
tantek approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member
@tantek tantek left a comment

Choose a reason for hiding this comment

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

from reading the diffs, these look like many good improvements. now we just need (regression) tests :)

@gRegorLove
Copy link
Member Author

Any possibility we can merge this? I'm not sure how to set up unit tests with this older framework to mock requests. The test links above demonstrate the different states and most of the changes are actually in the mf-cleaner dependency.

@gRegorLove
Copy link
Member Author

I'm feeling pretty confident about this so going to merge with the two approvals. We can work on implementation tests going forward, particularly after #82.

@gRegorLove gRegorLove merged commit 0913afe into indieweb:master Sep 30, 2022
@gRegorLove gRegorLove deleted the issue99 branch September 30, 2022 23:38
@gRegorLove gRegorLove mentioned this pull request Nov 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tantek tantek tantek approved these changes

@jalcine jalcine jalcine approved these changes

@aaronpk aaronpk Awaiting requested review from aaronpk

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate h-card fails on nested h-cards and returns an error 500
3 participants
@gRegorLove @tantek @jalcine