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 multi-byline #24

Merged
merged 2 commits into from
May 6, 2019
Merged

Conversation

happycollision
Copy link
Contributor

This PR aims to allow multiple authors to be rendered on a post, which is currently not possible.

Note:
The original method of applying a "hovered" class is actually better,
because when controlling in js, you can debounce. But that js logic
didn't appear to make the transition to this template in the first
place, so this is a starting point at least.

How:
Fix typo in post.hbs
This typo would force the single byline every time. Now, multiple
bylines are possible to render.

Fix template for byline-multiple.hbs
There were lots of things in here that did not get converted to Ember's
flavor of HBS. I am not aware of a #foreach block, but it seemed
broken, so I replaced one with an #each block and just completely
removed the other one, since it would presumably merely print the
authors' names, which are available on the cards.

happycollision and others added 2 commits May 5, 2019 23:33
Note:
The original method of applying a "hovered" class is actually better,
because when controlling in js, you can debounce. But that js logic
didn't appear to make the transition to this template in the first
place, so this is a starting point at least.

How:
**Fix typo in `post.hbs`**
This typo would force the single byline every time. Now, multiple
bylines are possible to render.

**Fix template for `byline-multiple.hbs`**
There were lots of things in here that did not get converted to Ember's
flavor of HBS. I am not aware of a `#foreach` block, but it seemed
broken, so I replaced one with an `#each` block and just completely
removed the other one, since it would presumably merely print the
authors' names, which are available on the cards.
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Looks great 🎉 I'm going to merge this right away because it is a significant improvement from what it was like before (from the looks of it it seemed 100% broken 🙈)

@mansona
Copy link
Member

mansona commented May 6, 2019

I'm merging this even though there is a bit of an issue still, it does seem like the popups aren't working for the hover over on the multiple byline 🤔

I thought for the longest time that the Ghost demo didn't have any posts with multiple authors so we couldn't compare what it should look like but I found one https://demo.ghost.io/the-businessman-the-fisherman/ Let me know if there is anything that I can do to help to get this to work 👍

I added an example of a multiple byline to your PR so we can use it for testing

@mansona mansona merged commit 69fa9c8 into empress:master May 6, 2019
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.

None yet

2 participants