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

fix: byline, avatar and date in carousel block #455

Merged
merged 2 commits into from
May 5, 2020

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented May 3, 2020

All Submissions:

Changes proposed in this Pull Request:

The byline and avatar display logic in Homepage Posts has changed since Carousel was first introduced. This brings the two in sync, extracting the handling of both to a shared utilities file.

Closes #445

How to test the changes in this Pull Request:

  1. Add Carousel block to the editor, verify author, avatar and date display.
  2. Publish the post and verify the display is the same.
  3. Add one or more Homepage Posts blocks, verify the byline/date/avatar display is unchanged.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

This works for me as described.

The only issue I ran into was for Co-Authors Plus and the Article Carousel block on the front end -- only one author would show when multiples were assigned. I'm able to recreate this in master too, though, and the multiple authors display as expected in the editor.

I'll spin up a separate issue for the multiple authors on the front-end!

return [
...accumulator,
<span className="author vcard" key={ author.id }>
<a className="url fn n" href="#">
Copy link
Member

Choose a reason for hiding this comment

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

Can the anchor be removed? It doesn't look like its functional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added support for the actual author links in 4dedb72

export const formatAvatars = authorInfo =>
authorInfo.map( author => (
<span className="avatar author-avatar" key={ author.id }>
<a className="url fn n" href="#">
Copy link
Member

Choose a reason for hiding this comment

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

Can the anchor be removed? It doesn't look like its functional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added support for the actual author links in 4dedb72

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeffersonrabb jeffersonrabb merged commit 85b7865 into master May 5, 2020
@jeffersonrabb jeffersonrabb deleted the fix/carousel-author-display branch May 5, 2020 00:53
jeffersonrabb pushed a commit that referenced this pull request May 5, 2020
* refactor: alphabetization (#447)

* fix: patterns preview thumbnails (#453)

* fix: update editor selector for reordering columns (#435)

* feat: use swiper for non-amp carousels

* chore: remove amp libraries in non-amp requests

* feat: use swiper for non-amp carousels

* feat: implement all carousel features with swiper

* chore: fix phpcs syntax issues

* fix: prev/next buttons position

* fix: use large featured image in the editor

* refactor: data attributes for autoplay and delay

Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>

* fix: byline, avatar and date in carousel block (#455)

* fix: byline, avatar and date in carousel block

* feat: real author links for avatar and author name

Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>
Co-authored-by: Laurel <laurel.fulford@automattic.com>
matticbot pushed a commit that referenced this pull request May 5, 2020
# [1.4.0](v1.3.1...v1.4.0) (2020-05-05)

### Bug Fixes

* byline, avatar and date in carousel block ([#455](#455)) ([85b7865](85b7865))
* patterns preview thumbnails ([#453](#453)) ([3ca80e0](3ca80e0))
* update editor selector for reordering columns ([#435](#435)) ([396826a](396826a))

### Features

* use swiper for non-amp carousels ([94c0a2b](94c0a2b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Articles Carousel: author missing in the editor
4 participants