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

Make Site-Title a link #24725

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Aug 21, 2020

Description

Closes #24716
Turns Site Title into a link on the frontend.

Caveats

Site Title is not a link in the editor. I tried multiple ways to solve it, but none of them worked properly.

  1. If I'm using an a tag as tagName for RichText Block then it gives a warning about using an inline element
  2. Using a custom element as a tagName (a element wrapped in the tagName) caused usability issues. The a element hijacked content editing. No matter where I clicked in the text, it always put the caret at the start of the text. If I used pointerEvents: 'none' then it ignored the hover style.

Looking forward to suggestions.

How has this been tested?

  1. Add Site Title block
  2. Save
  3. Site Title should point to the homepage on the frontend

Types of changes

Breaking change? (It breaks styling by adding a new element)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ockham
Copy link
Contributor

ockham commented Aug 21, 2020

Solving the editor problem might be tricky; it might require some architectural changes.

Some thoughts:

  • It makes sense that RichText only accepts a block element as tagName -- it's an editable thing after all, and those tend to look like blocks 😄
  • We also know that the (inline) a must be inside of any block elements (h1, p, etc) (that's just how the web works).
  • Having a RichText that's used for a block like this is a conceivable use case for e.g. themes: We want control over formatting options of the block (hence RichText), but we also want it to be a link.
  • Consequently, maybe we need to add a (bool) prop to RichText that wraps its content in an a.

This will require changes to the RichText component, which is widely used. It might make sense merge this PR as-is, and tackle editing mode in a separate PR (so we don't block progress on this PR).

@david-szabo97
Copy link
Member Author

Yep, that was my thought as well - we might need to touch the RichText component. It would be awesome to add a contentContainer prop. Let's try to merge this as-is and circle back to the editor problem in another PR/issue - as you suggested.

@david-szabo97 david-szabo97 marked this pull request as ready for review August 24, 2020 08:37
@ockham
Copy link
Contributor

ockham commented Aug 24, 2020

Some e2e tests and JS unit tests are failing (which seems unrelated). I'll restart them.

@david-szabo97
Copy link
Member Author

Seems to fail because of this

    ["Browserslist: caniuse-lite is outdated. Please run the following command: `npm update`"]

@ockham
Copy link
Contributor

ockham commented Aug 24, 2020

Seems to fail because of this

    ["Browserslist: caniuse-lite is outdated. Please run the following command: `npm update`"]

I'll try a rebase.

@ockham ockham force-pushed the feature/make-site-title-a-link branch from 27c0abc to b6c2cdc Compare August 24, 2020 16:06
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

One note here. Let's also wait for CI to go green. Otherwise looks good to merge 👍

@ockham
Copy link
Contributor

ockham commented Aug 24, 2020

The e2e test errors are a bit weird, I'll try a rebase.

@ockham ockham force-pushed the feature/make-site-title-a-link branch from b6c2cdc to f38a927 Compare August 24, 2020 18:00
@vindl
Copy link
Member

vindl commented Aug 24, 2020

Solving the editor problem might be tricky; it might require some architectural changes.

@ockham is this something that we actually need or want to solve in the editor? What would the behavior be in that case? 🤔

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

This looks good to me! As a follow-up we might need to update the relevant FSE themes to preserve the existing look, since this will likely introduce underline for site title that previously wasn't there?

@ockham
Copy link
Contributor

ockham commented Aug 25, 2020

Solving the editor problem might be tricky; it might require some architectural changes.

@ockham is this something that we actually need or want to solve in the editor? What would the behavior be in that case?

That's a bit of TBD indeed. The Site Logo does something like that (<a /> tag and event handler with preventDefault IIRC -- so we're at least getting a different cursor, and the link target shows up in the browser's bottom status bar on mouse over), and I think we should represent faithfully in the editor that the site title is going to be a link.

Furthermore, I think there have been some designs on making those links better 'navigable' in the editor -- possible in a similar way as Google Docs (small popover with a few options). So I think it makes sense to pave the way 🤔

@ockham
Copy link
Contributor

ockham commented Aug 25, 2020

This looks good to me! As a follow-up we might need to update the relevant FSE themes to preserve the existing look, since this will likely introduce underline for site title that previously wasn't there?

Yeah, we'll probably need to do that.

@ockham
Copy link
Contributor

ockham commented Aug 25, 2020

Another weird e2e error, this time about a snapshot 🙄 I'll try another rebase.

@ockham ockham force-pushed the feature/make-site-title-a-link branch from e30047b to 6dc327b Compare August 25, 2020 13:21
@ockham ockham force-pushed the feature/make-site-title-a-link branch from 6dc327b to 1fc513e Compare August 25, 2020 13:26
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Tests are finally passing 👍

@ockham ockham merged commit 13f4293 into WordPress:master Aug 25, 2020
@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 25, 2020
@carolinan carolinan added the [Block] Site Title Affects the Site Title Block label Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Title Affects the Site Title Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site-Title should be a link
4 participants