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

feat(seo): Do not use h3 header for poster author in posts stream #3732

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

rob006
Copy link
Contributor

@rob006 rob006 commented Feb 11, 2023

See #3724 (comment)

However this is more tricky, as some extensions rely on current markup. During brief testing I found 2 problems:

  1. Best Answer extension uses h3 for displaying username and it relies on styles provided by core. That is why I kept old styles for h3 tag - additional styles should not be a big problem and we keep BC this way. Redundant styles could be removed on 2.0 release.
  2. FoF Gamification crashed completely, because it searches for h3 tag:
    https://github.com/FriendsOfFlarum/gamification/blob/aad90d175fe55b3609de8c7649f9364765a37853/js/src/forum/components/RankingsPage.js#L76
    This could be quite easily fixed in extension, but BC break is obvious and there could be more extensions affected by this change. If we want to keep BC here, I don't think there is other solution than postponing this change to 2.0.

In future it would be better to add CSS classes everywhere and discourage relying on markup, as used tag may always change. Even some basic cases like targeting link using a selector may be problematic, because in future this link may be changed to button instead.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@rob006 rob006 requested a review from a team as a code owner February 11, 2023 20:46
@SychO9
Copy link
Member

SychO9 commented Feb 15, 2023

Based on the PR description and on second thought, this would be better suited for 2.0 considering a lot of extensions add content next to the user name. If it was just about breaking styles in extensions that'd be fine, but JS is more problematic, and I'm pretty sure it's not just the FoF extensions mentioned.

@SychO9 SychO9 added this to the 2.0 milestone Feb 15, 2023
@SychO9 SychO9 added type/seo javascript Pull requests that update Javascript code labels Feb 15, 2023
@rob006
Copy link
Contributor Author

rob006 commented Feb 15, 2023

I could revert change of tag name, so it will be h3 again. But the rest of changes should be fine and I think it is worth to merge them to 1.x, so extensions authors could target elements using class name and prepare earlier for upcoming BC breaks.

@SychO9
Copy link
Member

SychO9 commented Feb 15, 2023

@rob006 agreed, let's do that!

@rob006
Copy link
Contributor Author

rob006 commented Feb 15, 2023

Done.

@luceos luceos merged commit fee6ffe into flarum:main Feb 15, 2023
@luceos luceos modified the milestones: 2.0, 1.7 Feb 15, 2023
@rob006 rob006 deleted the seo-headings-part-3 branch February 15, 2023 17:49
@luceos luceos mentioned this pull request Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/seo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants